On Tue, Oct 29, 2024 at 3:18 PM vignesh C <vignes...@gmail.com> wrote: > > On Tue, 29 Oct 2024 at 11:30, Peter Smith <smithpb2...@gmail.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.
