Hi. Here are my v32-0002 review comments:
======
src/backend/replication/logical/tablesync.c
1. fetch_remote_table_info
/*
- * Get column lists for each relation.
+ * Get column lists for each relation, and check if any of the
+ * publications have the 'publish_generated_columns' parameter enabled.
I am not 100% sure about this logic anymore. Maybe it is OK, but it
requires careful testing because with Amit's "column lists take
precedence" it is now possible for the publication to say
'publish_generated_columns=false', but the publication can still
publish gencols *anyway* if they were specified in a column list.
~~~
2.
/*
* Fetch info about column lists for the relation (from all the
* publications).
*/
+ StringInfo pub_names = makeStringInfo();
+
+ get_publications_str(MySubscription->publications, pub_names, true);
resetStringInfo(&cmd);
appendStringInfo(&cmd,
~
nit - The comment here seems misplaced.
~~~
3.
+ if (server_version >= 120000)
+ {
+ has_pub_with_pubgencols = server_version >= 180000 && has_pub_with_pubgencols;
+
+ if (!has_pub_with_pubgencols)
+ appendStringInfo(&cmd, " AND a.attgenerated = ''");
+ }
My previous review comment about this [1 #10] was:
Can the 'gencols_allowed' var be removed, and the condition just be
replaced with if (!has_pub_with_pubgencols)? It seems equivalent
unless I am mistaken.
nit - So the current v32 code is not what I was expecting. What I
meant was 'has_pub_with_pubgencols' can only be true if server_version
>= 180000, so I thought there was no reason to check it again. For
reference, I've changed it to like I meant in the nitpicks attachment.
Please see if that works the same.
======
[1] my review of v31-0002.
https://www.postgresql.org/message-id/CAHut%2BPusbhvPrL1uN1TKY%3DFd4zu3h63eDebZvsF%3Duy%2BLBKTwgA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index 0e34d7c..9fed6b3 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -912,13 +912,12 @@ fetch_remote_table_info(char *nspname, char *relname,
bool **remotegenlist_res,
WalRcvExecResult *pubres;
TupleTableSlot *tslot;
Oid attrsRow[] = {INT2VECTOROID};
+ StringInfo pub_names = makeStringInfo();
/*
* Fetch info about column lists for the relation (from all the
* publications).
*/
- StringInfo pub_names = makeStringInfo();
-
get_publications_str(MySubscription->publications, pub_names,
true);
resetStringInfo(&cmd);
appendStringInfo(&cmd,
@@ -1047,13 +1046,8 @@ fetch_remote_table_info(char *nspname, char *relname,
bool **remotegenlist_res,
" WHERE a.attnum > 0::pg_catalog.int2"
" AND NOT a.attisdropped",
lrel->remoteid);
- if (server_version >= 120000)
- {
- has_pub_with_pubgencols = server_version >= 180000 &&
has_pub_with_pubgencols;
-
- if (!has_pub_with_pubgencols)
- appendStringInfo(&cmd, " AND a.attgenerated = ''");
- }
+ if (!has_pub_with_pubgencols)
+ appendStringInfo(&cmd, " AND a.attgenerated = ''");
appendStringInfo(&cmd,
" AND a.attrelid = %u"
