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