Re: Pgoutput not capturing the generated columns

On Thu, Oct 24, 2024 at 12:15 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> The attached v41 version patch has the changes for the same.
>

Please find comments for the new version as follows:
1.
+ Generated columns may be skipped during logical replication
according to the
+ <command>CREATE PUBLICATION</command> option
+ <link linkend="sql-createpublication-params-with-publish-generated-columns">
+ <literal>include_generated_columns</literal></link>.

The above statement doesn't sound to be clear. Can we change it to:
"Generated columns are allowed to be replicated during logical
replication according to the <command>CREATE PUBLICATION</command>
option .."?

2.
static void publication_invalidation_cb(Datum arg, int cacheid,
uint32 hashvalue);
-static void send_relation_and_attrs(Relation relation, TransactionId xid,
- LogicalDecodingContext *ctx,
- Bitmapset *columns);
static void send_repl_origin(LogicalDecodingContext *ctx,
...
...
static RelationSyncEntry *get_rel_sync_entry(PGOutputData *data,
Relation relation);
+static void send_relation_and_attrs(Relation relation, TransactionId xid,
+ LogicalDecodingContext *ctx,
+ RelationSyncEntry *relentry);

Why the declaration of this function is changed?

3.
+ /*
+ * Skip publishing generated columns if the option is not specified or
+ * if they are not included in the column list.
+ */
+ if (att->attgenerated && !relentry->pubgencols && !columns)

In the comment above, shouldn't "specified or" be "specified and"?

4.
+pgoutput_pubgencol_init(PGOutputData *data, List *publications,
+ RelationSyncEntry *entry)

{
...
+ foreach(lc, publications)
+ {
+ Publication *pub = lfirst(lc);
+
+ /* No need to check column list publications */
+ if (is_column_list_publication(pub, entry->publish_as_relid))

Are we ignoring column_list publications because for such publications
the value of column_list prevails and we ignore
'publish_generated_columns' value? If so, it is not clear from the
comments.

5.
/* Initialize the column list */
pgoutput_column_list_init(data, rel_publications, entry);
+
+ /* Initialize publish generated columns value */
+ pgoutput_pubgencol_init(data, rel_publications, entry);
+
+ /*
+ * Check if there is conflict with the columns selected for the
+ * publication.
+ */
+ check_conflicting_columns(rel_publications, entry);
}

It looks odd to check conflicting column lists among publications
twice once in pgoutput_column_list_init() and then in
check_conflicting_columns(). Can we merge those?

--
With Regards,
Amit Kapila.