Hi, here are my review comments for patch v31-0002.
======
1. General.
IMO patches 0001 and 0002 should be merged when next posted. IIUC the
reason for the split was only because there were 2 different authors
but that seems to be not relevant anymore.
======
Commit message
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, we need to copy using the syntax:
'COPY (SELECT column_name FROM table_name) TO STDOUT'.
~
2a.
Should clarify that 'copy_data' is a SUBSCRIPTION parameter.
2b.
Should clarify that 'publish_generated_columns' is a PUBLICATION parameter.
======
src/backend/replication/logical/tablesync.c
make_copy_attnamelist:
3.
- for (i = 0; i < rel->remoterel.natts; i++)
+ desc = RelationGetDescr(rel->localrel);
+ localgenlist = palloc0(rel->remoterel.natts * sizeof(bool));
Each time I review this code I am tricked into thinking it is wrong to
use rel->remoterel.natts here for the localgenlist. AFAICT the code is
actually fine because you do not store *all* the subscriber gencols in
'localgenlist' -- you only store those with matching names on the
publisher table. It might be good if you could add an explanatory
comment about that to prevent any future doubts.
~~~
4.
+ if (!remotegenlist[remote_attnum])
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("logical replication target relation \"%s.%s\" has a
generated column \"%s\" "
+ "but corresponding column on source relation is not a generated column",
+ rel->remoterel.nspname, rel->remoterel.relname, NameStr(attr->attname))));
This error message has lots of good information. OTOH, I think when
copy_data=false the error would report the subscriber column just as
"missing", which is maybe less helpful. Perhaps that other
copy_data=false "missing" case can be improved to share the same error
message that you have here.
~~~
fetch_remote_table_info:
5.
IIUC, this logic needs to be more sophisticated to handle the case
that was being discussed earlier with Sawada-san [1]. e.g. when the
same table has gencols but there are multiple subscribed publications
where the 'publish_generated_columns' parameter differs.
Also, you'll need test cases for this scenario, because it is too
difficult to judge correctness just by visual inspection of the code.
~~~~
6.
nit - Change 'hasgencolpub' to 'has_pub_with_pubgencols' for
readability, and initialize it to 'false' to make it easy to use
later.
~~~
7.
- * Get column lists for each relation.
+ * Get column lists for each relation and check if any of the publication
+ * has generated column option.
and
+ /* Check if any of the publication has generated column option */
+ if (server_version >= 180000)
nit - tweak the comments to name the publication parameter properly.
~~~
8.
foreach(lc, MySubscription->publications)
{
if (foreach_current_index(lc) > 0)
appendStringInfoString(&pub_names, ", ");
appendStringInfoString(&pub_names, quote_literal_cstr(strVal(lfirst(lc))));
}
I know this is existing code, but shouldn't all this be done by using
the purpose-built function 'get_publications_str'
~~~
9.
+ ereport(ERROR,
+ errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not fetch gencolumns information from publication list: %s",
+ pub_names.data));
and
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("failed to fetch tuple for gencols from publication list: %s",
+ pub_names.data));
nit - /gencolumns information/generated column publication
information/ to make the errmsg more human-readable
~~~
10.
+ bool gencols_allowed = server_version >= 180000 && hasgencolpub;
+
+ if (!gencols_allowed)
+ appendStringInfo(&cmd, " AND a.attgenerated = ''");
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.
======
Please refer to the attachment which implements some of the nits
mentioned above.
======
[1]
https://www.postgresql.org/message-id/CAD21AoBun9crSWaxteMqyu8A_zme2ppa2uJvLJSJC2E3DJxQVA%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 723c44c..6d17984 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -850,7 +850,7 @@ fetch_remote_table_info(char *nspname, char *relname, bool
**remotegenlist_res,
Oid qualRow[] = {TEXTOID};
bool isnull;
bool *remotegenlist;
- bool hasgencolpub;
+ bool has_pub_with_pubgencols = false;
int natt;
ListCell *lc;
Bitmapset *included_cols = NULL;
@@ -897,8 +897,8 @@ fetch_remote_table_info(char *nspname, char *relname, bool
**remotegenlist_res,
/*
- * Get column lists for each relation and check if any of the
publication
- * has generated column option.
+ * Get column lists for each relation, and check if any of the
publications
+ * have the 'publish_generated_columns' parameter enabled.
*
* We need to do this before fetching info about column names and types,
* so that we can skip columns that should not be replicated.
@@ -989,7 +989,10 @@ fetch_remote_table_info(char *nspname, char *relname, bool
**remotegenlist_res,
walrcv_clear_result(pubres);
- /* Check if any of the publication has generated column option
*/
+ /*
+ * Check if any of the publications have the
'publish_generated_columns'
+ * parameter enabled.
+ */
if (server_version >= 180000)
{
WalRcvExecResult *gencolres;
@@ -1006,17 +1009,17 @@ fetch_remote_table_info(char *nspname, char *relname,
bool **remotegenlist_res,
if (gencolres->status != WALRCV_OK_TUPLES)
ereport(ERROR,
errcode(ERRCODE_CONNECTION_FAILURE),
- errmsg("could not fetch
gencolumns information from publication list: %s",
+ errmsg("could not fetch
generated column publication information from publication list: %s",
pub_names.data));
tslot = MakeSingleTupleTableSlot(gencolres->tupledesc,
&TTSOpsMinimalTuple);
if (!tuplestore_gettupleslot(gencolres->tuplestore,
true, false, tslot))
ereport(ERROR,
errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("failed to fetch tuple
for gencols from publication list: %s",
+ errmsg("failed to fetch tuple
for generated column publication information from publication list: %s",
pub_names.data));
- hasgencolpub = DatumGetBool(slot_getattr(tslot, 1,
&isnull));
+ has_pub_with_pubgencols =
DatumGetBool(slot_getattr(tslot, 1, &isnull));
Assert(!isnull);
ExecClearTuple(tslot);
@@ -1048,7 +1051,7 @@ fetch_remote_table_info(char *nspname, char *relname,
bool **remotegenlist_res,
if (server_version >= 120000)
{
- bool gencols_allowed = server_version >= 180000 &&
hasgencolpub;
+ bool gencols_allowed = server_version >= 180000 &&
has_pub_with_pubgencols;
if (!gencols_allowed)
appendStringInfo(&cmd, " AND a.attgenerated = ''");
