Re: A reloption for partitioned tables - parallel_workers

On Tue, Feb 16, 2021 at 3:05 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Feb 16, 2021 at 1:35 AM Seamus Abshere <seamus@abshere.net> wrote:
> > Here we go, my first patch... solves
https://www.postgresql.org/message-id/7d6fdc20-857c-4cbe-ae2e-c0ff9520ed55@www.fastmail.com
>
> Thanks for sending the patch here.
>
> It seems you haven't made enough changes for reloptions code to
> recognize parallel_workers as valid for partitioned tables, because
> even with the patch applied, I get this:
>
> create table rp (a int) partition by range (a);
> create table rp1 partition of rp for values from (minvalue) to (0);
> create table rp2 partition of rp for values from (0) to (maxvalue);
> alter table rp set (parallel_workers = 1);
> ERROR:  unrecognized parameter "parallel_workers"
>
> You need this:
>
> diff --git a/src/backend/access/common/reloptions.c
> b/src/backend/access/common/reloptions.c
> index 029a73325e..9eb8a0c10d 100644
> --- a/src/backend/access/common/reloptions.c
> +++ b/src/backend/access/common/reloptions.c
> @@ -377,7 +377,7 @@ static relopt_int intRelOpts[] =
>         {
>             "parallel_workers",
>             "Number of parallel processes that can be used per
> executor node for this relation.",
> -           RELOPT_KIND_HEAP,
> +           RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
>             ShareUpdateExclusiveLock
>         },
>         -1, 0, 1024
>
> which tells reloptions parsing code that parallel_workers is
> acceptable for both heap and partitioned relations.
>
> Other comments on the patch:
>
> * This function calling style, where the first argument is not placed
> on the same line as the function itself, is not very common in
> Postgres:
>
> +       /* First see if there is a root-level setting for parallel_workers */
> +       parallel_workers = compute_parallel_worker(
> +           rel,
> +           -1,
> +           -1,
> +           max_parallel_workers_per_gather
> +
>
> This makes the new code look very different from the rest of the
> codebase.  Better to stick to existing styles.
>
> 2. It might be a good idea to use this opportunity to add a function,
> say compute_append_parallel_workers(), for the code that does what the
> function name says.  Then the patch will simply add the new
> compute_parallel_worker() call at the top of that function.
>
> 3. I think we should consider the Append parent relation's
> parallel_workers ONLY if it is a partitioned relation, because it
> doesn't make a lot of sense for other types of parent relations.  So
> the new code should look like this:
>
> if (IS_PARTITIONED_REL(rel))
>     parallel_workers = compute_parallel_worker(rel, -1, -1,
> max_parallel_workers_per_gather);
>
> 4. Maybe it also doesn't make sense to consider the parent relation's
> parallel_workers if Parallel Append is disabled
> (enable_parallel_append = off).  That's because with a simple
> (non-parallel) Append running under Gather, all launched parallel
> workers process the same partition before moving to the next one.
> OTOH, one's intention of setting parallel_workers on the parent
> partitioned table would most likely be to distribute workers across
> partitions, which is only possible with parallel Append
> (enable_parallel_append = on).  So, the new code should look like
> this:
>
> if (IS_PARTITIONED_REL(rel) && enable_parallel_append)
>     parallel_workers = compute_parallel_worker(rel, -1, -1,
> max_parallel_workers_per_gather);

Here is an updated version of the Seamus' patch that takes into
account these and other comments received on this thread so far.
Maybe warrants adding some tests too but I haven't.

Seamus, please register this patch in the next commit-fest:
https://commitfest.postgresql.org/32/

If you haven't already, you will need to create a community account to
use that site.

-- 
Amit Langote
EDB: http://www.enterprisedb.com