Hello,
On Sun, Oct 6, 2019 at 9:48 PM Nikolay Shaplov <dhyan@nataraj.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
