On Tue, Feb 23, 2021 at 7:24 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.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