Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options

Hello,

On Sun, Oct 6, 2019 at 9:48 PM Nikolay Shaplov <dhyan(at)nataraj(dot)su> wrote:
> This message is follow up to the "Get rid of the StdRdOptions" patch thread:
> https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m
>
> I've split patch into even smaller parts and commitfest want each patch in
> separate thread. So it is new thread.
>
> The idea of this patch is following: If you read the code, partitioned tables
> do not have any options (you will not find RELOPT_KIND_PARTITIONED in
> boolRelOpts, intRelOpts, realRelOpts, stringRelOpts and enumRelOpts in
> reloption.c), but it uses StdRdOptions to store them (these no options).
>
> If partitioned table is to have it's own option set (even if it is empty now)
> it would be better to save it into separate structure, like it is done for
> Views, not adding them to StdRdOptions, like current code hints to do.
>
> So in this patch I am creating a structure that would store partitioned table
> options (it is empty for now as there are no options for this relation kind),
> and all other code that would use this structure as soon as the first option
> comes.
>
> I think it is bad idea to suggest option adder to ad it to StdRdOption, we
> already have a big mess there. Better if he add it to an new empty structure.

I tend to agree that this improves readability of the reloptions code a bit.

Some comments on the patch:

How about naming the function partitioned_table_reloptions() instead
of partitioned_reloptions()?

+ * Option parser for partitioned relations

Since partitioned_reloptions only caters to partitioned "tables",
maybe use "tables" here instead of "relations".

+ /*
+ * Since there is no options for patitioned table for now, we just do
+ * validation to report incorrect option error and leave.
+ */

Fix typo and minor rewording:

"Since there are no options for partitioned tables..."

+ switch ((int)relkind)

Needs a space between (int) and relkind, but I don't think the (int)
cast is really necessary. I don't see it in other places.

+ * Binary representation of relation options for Partitioned relations.

"for partitioned tables".

Speaking of partitioned relations vs tables, I see that you didn't
touch partitioned indexes (RELKIND_PARTITIONED_INDEX relations). Is
that because we leave option handling to the index AMs?

Thanks,
Amit