Re: Pgoutput not capturing the generated columns

On Fri, Oct 25, 2024 at 12:07 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Oct 24, 2024 at 8:50 PM vignesh C <vignesh21(at)gmail(dot)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.