On Fri, Oct 25, 2024 at 3:54 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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.". >
Fixed. > 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." > Fixed. > 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." > Fixed. The v43 version patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CAHv8RjJJJRzy83tG0nB90ivYcp7sFKTU%3D_BcQ-nUZ7VbHFwceA%40mail.gmail.com Thanks and Regards, Shubham Khanna.
