On 16/09/2016 20:24, Robert Haas wrote:
> On Wed, Jun 29, 2016 at 10:46 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> Your patch looks good to me and is ready for a committer's look.
>>
>> Notes for committer -
>> a. Verify if description of newly added Guc max_parallel_workers looks
>> okay to you, me and Julien are not in 100% agreement on that.
>> b. Comments might need some improvement.
>
> This patch needs to be rebased. I hope somebody can volunteer to do
> that, because I'd like to commit it once we've hashed out the details.
>
I just rebased the previous patch on current HEAD, with some other
modifications, see below (attached v8 if that helps).
> Would it bother anybody very much if we bumped up these values, say by
> increasing max_worker_processes from 8 to 16 and max_parallel_workers
> from 4 (as it is in the current patch version) to 8? I feel like 4 is
> a bit more conservative than I'd like to be by default, and I'd like
> to make sure that we leave room for other sorts of background workers
> between the two limits.
>
That's fine by me. Should this be done (if there's no objection) in the
same patch, or in another one?
> I'd suggest renaming the "parallel" flag to BackgroundWorkerSlot to
> "is_parallel_worker". Or, actually, what I think would be better is
> to give it a name like worker_class, and then we can have
> BGWORKER_CLASS_PARALLEL and perhaps eventually
> BGWORKER_CLASS_REPLICATION, etc.
>
For now I just renamed "parallel" to "is_parallel_worker", since this is
straightforward. For a new "worker_class", I guess we'd need a new enum
stored in BackgroundWorker struct instead of the
BGWORKER_IS_PARALLEL_WORKER flag, and store it in the
BackgroundWorkerSlot. Should I do that instead?
> + * terminated ones. These counters can of course overlaps, but it's not
> + * important here since the substraction will still give the right number.
>
> overlaps -> overflow. substraction -> subtraction.
>
oops sorry, fixed
> + /*
> + * We need a write barrier to make sure the update of
> + * parallel_terminate_count is done before the store to in_use
> + */
>
> Does the order actually matter here?
>
After some more thinking, it looks like a reorder here won't have any
impact. I'll remove it, unless Amit has an objection about it.
> + {"max_parallel_workers", PGC_USERSET, RESOURCES_ASYNCHRONOUS,
> + gettext_noop("Sets the maximum number of
> parallel processes for the cluster."),
>
> I suggest: sets the maximum number of parallel workers that can be
> active at one time.
>
changed
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org