On Fri, Oct 25, 2024 at 12:07 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Oct 24, 2024 at 8:50 PM vignesh C <vignes...@gmail.com> wrote: > > > > The v42 version patch attached at [1] has the changes for the same. > > > > Some more comments: >
1. +pgoutput_pubgencol_init(PGOutputData *data, List *publications, + RelationSyncEntry *entry) Can we name it as check_and_init_gencol? I don't know if it is a good idea to append a prefix pgoutput for local functions. It is primarily used for exposed functions from pgoutput.c. I see that in a few cases we do that for local functions as well but that is not a norm. A related point: + /* Initialize publish generated columns value */ + pgoutput_pubgencol_init(data, rel_publications, entry); Accordingly change this comment to something like: "Check whether to publish to generated columns.". 2. +/* + * Returns true if the relation has column list associated with the + * publication, false if the relation has no column list associated with the + * publication. + */ +bool +is_column_list_publication(Publication *pub, Oid relid) ... ... How about naming the above function as has_column_list_defined()? Also, you can write the above comment as: "Returns true if the relation has column list associated with the publication, false otherwise." 3. + /* + * The column list takes precedence over pubgencols, so skip checking + * column list publications. + */ + if (is_column_list_publication(pub, entry->publish_as_relid)) Let's change this comment to: "The column list takes precedence over publish_generated_columns option. Those will be checked later, see pgoutput_column_list_init." -- With Regards, Amit Kapila.
