Re: A reloption for partitioned tables - parallel_workers

On Tue, Feb 23, 2021 at 7:24 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > It seems the patch does not include the code that get the
> > > parallel_workers from new struct " PartitionedTableRdOptions ", Did I miss
> > something ?
> >
> > Aren't the following hunks in the v2 patch what you meant?
> >
> > diff --git a/src/backend/access/common/reloptions.c
> > b/src/backend/access/common/reloptions.c
> > index c687d3ee9e..f8443d2361 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
> > @@ -1962,12 +1962,18 @@ bytea *
> > partitioned_table_reloptions(Datum reloptions, bool validate) {
> > /*
> > - * There are no options for partitioned tables yet, but this is able to do
> > - * some validation.
> > + * Currently the only setting known to be useful for partitioned tables
> > + * is parallel_workers.
> > */
> > + static const relopt_parse_elt tab[] = { {"parallel_workers",
> > + RELOPT_TYPE_INT, offsetof(PartitionedTableRdOptions,
> > + parallel_workers)}, };
> > +
> > return (bytea *) build_reloptions(reloptions, validate,
> > RELOPT_KIND_PARTITIONED,
> > - 0, NULL, 0);
> > + sizeof(PartitionedTableRdOptions),
> > + tab, lengthof(tab));
> > }
> >
> > /*
> >
> > diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index
> > 10b63982c0..fe114e0856 100644
> > --- a/src/include/utils/rel.h
> > +++ b/src/include/utils/rel.h
> > @@ -308,6 +308,16 @@ typedef struct StdRdOptions
> > bool vacuum_truncate; /* enables vacuum to truncate a relation */ }
> > StdRdOptions;
> >
> > +/*
> > + * PartitionedTableRdOptions
> > + * Contents of rd_options for partitioned tables */ typedef struct
> > +PartitionedTableRdOptions {
> > + int32 vl_len_; /* varlena header (do not touch directly!) */ int
> > +parallel_workers; /* max number of parallel workers */ }
> > +PartitionedTableRdOptions;
> > +
> > #define HEAP_MIN_FILLFACTOR 10
> > #define HEAP_DEFAULT_FILLFACTOR 100
> Hi,
>
> I am not sure.
> IMO, for normal table, we use the following macro to get the parallel_workers:
> ----------------------
> /*
> * RelationGetParallelWorkers
> * Returns the relation's parallel_workers reloption setting.
> * Note multiple eval of argument!
> */
> #define RelationGetParallelWorkers(relation, defaultpw) \
> ((relation)->rd_options ? \
> ((StdRdOptions *) (relation)->rd_options)->parallel_workers : (defaultpw))
> ----------------------
>
> Since we add new struct " PartitionedTableRdOptions ", It seems we need to get parallel_workers in different way.
> Do we need similar macro to get partitioned table's parallel_workers ?

Oh, you're right. The parallel_workers setting of a relation is only
accessed through this macro, even for partitioned tables, and I can
see that it is actually wrong to access a partitioned table's
parallel_workers through this macro as-is. Although I hadn't tested
that, so thanks for pointing that out.

> Like:
> #define PartitionedTableGetParallelWorkers(relation, defaultpw) \
> xxx
> (PartitionedTableRdOptions *) (relation)->rd_options)->parallel_workers : (defaultpw))

I'm thinking it would be better to just modify the existing macro to
check relkind to decide which struct pointer type to cast the value in
rd_options to.

I will post an updated patch later.

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