Re: Pgoutput not capturing the generated columns

On Tue, Oct 29, 2024 at 3:18 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 29 Oct 2024 at 11:30, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Here are my review comments for patch v44-0002.
> >
> > ======
> > Commit message.
> >
> > 1.
> > The commit message is missing.
>
> This patch is now merged, so no change required.
>
> > ======
> > src/backend/replication/logical/tablesync.c
> >
> > fetch_remote_table_info:
> >
> > 2.
> > +fetch_remote_table_info(char *nspname, char *relname, LogicalRepRelation *lrel,
> > + List **qual, bool *remotegencolpresent)
> >
> > The name 'remotegencolpresent' sounds like it means a generated col is
> > present in the remote table, but don't we only care when it is being
> > published? So, would a better parameter name be more like
> > 'remote_gencol_published'?
>
> I have changed it to gencol_published based on Amit's suggestion at [1].
>
> > ~~~
> >
> > 3.
> > Would it be better to introduce a new human-readable variable like:
> > bool check_for_published_gencols = (server_version >= 180000);
> >
> > because then you could use that instead of having the 180000 check in
> > multiple places.
>
> I felt this is not required, so not making any change for this.
>
> > ~~~
> >
> > 4.
> > - lengthof(attrRow), attrRow);
> > + server_version >= 180000 ? lengthof(attrRow) : lengthof(attrRow) -
> > 1, attrRow);
> >
> > If you wish, that length calculation could be written more concisely like:
> > lengthof(attrow) - (server_version >= 180000 ? 0 : 1)
>
> I felt the current one is better, also Amit feels the same way as in
> [1]. Not making any change for this.
>
> > ~~~
> >
> > 5.
> > + if (server_version >= 180000)
> > + *remotegencolpresent |= DatumGetBool(slot_getattr(slot, 5, &isnull));
> > +
> >
> > Should this also say Assert(!isnull)?
>
> Added an assert
>
> > ======
> > src/test/subscription/t/031_column_list.pl
> >
> > 6.
> > + qq(0|1),
> > + 'replication with generated columns in column list');
> >
> > Perhaps this message should be worded slightly differently, to
> > distinguish it from the "normal" replication message.
> >
> > /replication with generated columns in column list/initial replication
> > with generated columns in column list/
>
> Modified
>
> The v45 version patch attached at [2] has the changes for the same.
>
> [1] - https://www.postgresql.org/message-id/CAA4eK1Lpzy3eqd2AOM%2BTXp80SFL1cCfX3cf9thjL-hJxn%2BAYGA%40mail.gmail.com
> [2] - https://www.postgresql.org/message-id/CALDaNm1oc-%2Buav380Z1k6gCZY5GJn5ZYKRexwM%2BqqGiRinUS-Q%40mail.gmail.com
>

While performing the Backward Compatibility Test, I found that
'tablesync' is not working for the older versions i.e., from
version-12 till version-15.
I created 2 nodes ; PUBLISHER on old versions and SUBSCRIBER on HEAD +
v45 Patch for testing.
Following was done on the PUBLISHER node:
CREATE TABLE t1 (c1 int, c2 int GENERATED ALWAYS AS (c1 * 2) STORED);
INSERT INTO t1 (c1) VALUES (1), (2);
CREATE PUBLICATION pub1 for table t1;

Following was done on the SUBSCRIBER node:
CREATE TABLE t1 (c1 int, c2 int);
CREATE SUBSCRIPTION sub1 CONNECTION 'dbname=postgres' PUBLICATION pub1;

Following Error occurs repeatedly in the Subscriber log files:
ERROR: could not start initial contents copy for table "public.t1":
ERROR: column "c2" is a generated column
DETAIL: Generated columns cannot be used in COPY.

Thanks and Regards,
Shubham Khanna.