Hi, here are my review comments for patch v37-0001.
======
Commit message
1.
Example usage of subscription option:
CREATE PUBLICATION FOR TABLE tab_gencol WITH (publish_generated_columns
= true);
~
This is wrong -- it's not a "subscription option". Better to just say
"Example usage:"
~~~
2.
When 'copy_data' is true, during the initial sync, the data is replicated from
the publisher to the subscriber using the COPY command. The normal COPY
command does not copy generated columns, so when 'publish_generated_columns'
is true...
~
By only mentioning the "when ... is true" case this description does
not cover the scenario when 'publish_generated_columns' is false when
the publication column list has a generated column.
~~~
3.
typo - /replication of generated column/replication of generated columns/
typo - /filed/filled/
typo - 'pg_publicataion' catalog
======
src/backend/replication/logical/tablesync.c
make_copy_attnamelist:
4.
nit - missing word in a comment
~~~
fetch_remote_table_info:
5.
+ appendStringInfo(&cmd,
" FROM pg_catalog.pg_attribute a"
" LEFT JOIN pg_catalog.pg_index i"
" ON (i.indexrelid = pg_get_replica_identity_index(%u))"
" WHERE a.attnum > 0::pg_catalog.int2"
- " AND NOT a.attisdropped %s"
+ " AND NOT a.attisdropped", lrel->remoteid);
+
+ appendStringInfo(&cmd,
" AND a.attrelid = %u"
" ORDER BY a.attnum",
- lrel->remoteid,
- (walrcv_server_version(LogRepWorkerWalRcvConn) >= 120000 ?
- "AND a.attgenerated = ''" : ""),
lrel->remoteid);
Version v37-0001 has removed a condition previously between these two
appendStringInfo's. But, that now means there is no reason to keep
these statements separated. These should be combined now to use one
appendStringInfo.
~
6.
+ if (server_version >= 120000)
+ remotegenlist[natt] = DatumGetBool(slot_getattr(slot, 5, &isnull));
+
Are you sure the version check for 120000 is correct? IIUC, this 5
matches the 'attgenerated' column, but the SQL for that was
constructed using a different condition:
if (server_version >= 180000)
appendStringInfo(&cmd, ", a.attgenerated != ''");
It is this 120000 versus 180000 difference that makes me suspicious of
a potential mistake.
~~~
7.
+ /*
+ * If the column is generated and neither the generated column option
+ * is specified nor it appears in the column list, we will skip it.
+ */
+ if (remotegenlist[natt] && !has_pub_with_pubgencols &&
+ !bms_is_member(attnum, included_cols))
+ {
+ ExecClearTuple(slot);
+ continue;
+ }
7b.
I am also suspicious about how this condition interacts with the other
condition (shown below) that came earlier:
/* If the column is not in the column list, skip it. */
if (included_cols != NULL && !bms_is_member(attnum, included_cols))
Something doesn't seem right. e.g. If we can only get here by passing
the earlier condition, then it means we already know the generated
condition was *not* a member of a column list.... in which case that
should affect this new condition and the new comment too.
======
src/backend/replication/pgoutput/pgoutput.c
pgoutput_column_list_init:
8.
/*
- * If the publication is FOR ALL TABLES then it is treated the same as
- * if there are no column lists (even if other publications have a
- * list).
+ * Process potential column lists for the following cases: a. Any
+ * publication that is not FOR ALL TABLES. b. When the publication is
+ * FOR ALL TABLES and 'publish_generated_columns' is false. FOR ALL
+ * TABLES publication doesn't have user-defined column lists, so all
+ * columns will be replicated by default. However, if
+ * 'publish_generated_columns' is set to false, column lists must
+ * still be created to exclude any generated columns from being
+ * published.
*/
nit - please reformat this comment so the bullets are readable
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index 6f9e126..365f987 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -693,7 +693,7 @@ process_syncing_tables(XLogRecPtr current_lsn)
}
/*
- * Create list of columns for COPY based on logical relation mapping.
+ * Create a list of columns for COPY based on logical relation mapping.
* Exclude columns that are subscription table generated columns.
*/
static List *
@@ -749,7 +749,7 @@ make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool
*remotegenlist)
}
/*
- * Construct column list for COPY, excluding columns that are
subscription
+ * Construct a column list for COPY, excluding columns that are
subscription
* table generated columns.
*/
for (int i = 0; i < rel->remoterel.natts; i++)
diff --git a/src/backend/replication/pgoutput/pgoutput.c
b/src/backend/replication/pgoutput/pgoutput.c
index d953a1a..6d6032d 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1073,13 +1073,15 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
Bitmapset *cols = NULL;
/*
- * Process potential column lists for the following cases: a.
Any
- * publication that is not FOR ALL TABLES. b. When the
publication is
- * FOR ALL TABLES and 'publish_generated_columns' is false. FOR
ALL
- * TABLES publication doesn't have user-defined column lists,
so all
- * columns will be replicated by default. However, if
- * 'publish_generated_columns' is set to false, column lists
must
- * still be created to exclude any generated columns from being
+ * Process potential column lists for the following cases:
+ *
+ * a. Any publication that is not FOR ALL TABLES.
+ *
+ * b. When the publication is FOR ALL TABLES and
+ * 'publish_generated_columns' is false. FOR ALL TABLES
publication doesn't
+ * have user-defined column lists, so all columns will be
replicated by
+ * default. However, if 'publish_generated_columns' is set to
false, column
+ * lists must still be created to exclude any generated columns
from being
* published.
*/
if (!(pub->alltables && pub->pubgencols))
