Hi, here are some review comments for patch v9-0002.
======
src/backend/replication/logical/relation.c
1. logicalrep_rel_open
- if (attr->attisdropped)
+ if (attr->attisdropped ||
+ (!MySubscription->includegencols && attr->attgenerated))
You replied to my question from the previous review [1, #2] as follows:
In case 'include_generated_columns' is 'true'. column list in
remoterel will have an entry for generated columns. So, in this case
if we skip every attr->attgenerated, we will get a missing column
error.
~
TBH, the reason seems very subtle to me. Perhaps that
"(!MySubscription->includegencols && attr->attgenerated))" condition
should be coded as a separate "if", so then you can include a comment
similar to your answer, to explain it.
======
src/backend/replication/logical/tablesync.c
make_copy_attnamelist:
NITPICK - punctuation in function comment
NITPICK - add/reword some more comments
NITPICK - rearrange comments to be closer to the code they are commenting
~
2. make_copy_attnamelist.
+ /*
+ * Construct column list for COPY.
+ */
+ for (int i = 0; i < rel->remoterel.natts; i++)
+ {
+ if(!gencollist[i])
+ attnamelist = lappend(attnamelist,
+ makeString(rel->remoterel.attnames[i]));
+ }
IIUC isn't this assuming that the attribute number (aka column order)
is the same on the subscriber side (e.g. gencollist idx) and on the
publisher side (e.g. remoterel.attnames[i]). AFAIK logical
replication does not require this ordering must be match like that,
therefore I am suspicious this new logic is accidentally imposing new
unwanted assumptions/restrictions. I had asked the same question
before [1-#4] about this code, but there was no reply.
Ideally, there would be more test cases for when the columns
(including the generated ones) are all in different orders on the
pub/sub tables.
~~~
3. General - varnames.
It would help with understanding if the 'attgenlist' variables in all
these functions are re-named to make it very clear that this is
referring to the *remote* (publisher-side) table genlist, not the
subscriber table genlist.
~~~
4.
+ int i = 0;
+
appendStringInfoString(&cmd, "COPY (SELECT ");
- for (int i = 0; i < lrel.natts; i++)
+ foreach_ptr(ListCell, l, attnamelist)
{
- appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
- if (i < lrel.natts - 1)
+ appendStringInfoString(&cmd, quote_identifier(strVal(l)));
+ if (i < attnamelist->length - 1)
appendStringInfoString(&cmd, ", ");
+ i++;
}
4a.
I think the purpose of the new macros is to avoid using ListCell, and
also 'l' is an unhelpful variable name. Shouldn't this code be more
like:
foreach_node(String, att_name, attnamelist)
~
4b.
The code can be far simpler if you just put the comma (", ") always
up-front except the *first* iteration, so you can avoid checking the
list length every time. For example:
if (i++)
appendStringInfoString(&cmd, ", ");
======
src/test/subscription/t/011_generated.pl
5. General.
Hmm. This patch 0002 included many formatting changes to tables tab2
and tab3 and subscriptions sub2 and sub3 but they do not belong here.
The proper formatting for those needs to be done back in patch 0001
where they were introduced. Patch 0002 should just concentrate only on
the new stuff for patch 0002.
~
6. CREATE TABLES would be better in pairs
IMO it will be better if the matching CREATE TABLE for pub and sub are
kept together, instead of separating them by doing all pub then all
sub. I previously made the same comment for patch 0001, so maybe it
will be addressed next time...
~
7. SELECT *
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT * FROM tab4 ORDER BY a");
It will be prudent to do explicit "SELECT a,b,c" instead of "SELECT
*", for readability and so there are no surprises.
======
99.
Please also refer to my attached nitpicks diff for numerous cosmetic
changes, and apply if you agree.
======
[1]
https://www.postgresql.org/message-id/CAHut%2BPtAsEc3PEB1KUk1kFF5tcCrDCCTcbboougO29vP1B4E2Q%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 0fbf661..ddeb6a8 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -694,7 +694,7 @@ process_syncing_tables(XLogRecPtr current_lsn)
/*
* Create list of columns for COPY based on logical relation mapping. Do not
- * include generated columns, of the subscription table, in the column list.
+ * include generated columns of the subscription table in the column list.
*/
static List *
make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool *attgenlist)
@@ -706,6 +706,7 @@ make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool
*attgenlist)
desc = RelationGetDescr(rel->localrel);
gencollist = palloc0(MaxTupleAttributeNumber * sizeof(bool));
+ /* Loop to handle subscription table generated columns. */
for (int i = 0; i < desc->natts; i++)
{
int attnum;
@@ -716,23 +717,25 @@ make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool
*attgenlist)
attnum = logicalrep_rel_att_by_name(&rel->remoterel,
NameStr(attr->attname));
-
- /*
- * Check if subscription table have a generated column with same
- * column name as a non-generated column in the corresponding
- * publication table.
- * 'gencollist' stores column if it is a generated column in
- * subscription table. We should not include this column in
column
- * list for COPY.
- */
if (attnum >= 0)
{
+ /*
+ * Check if the subscription table generated column has
same
+ * name as a non-generated column in the corresponding
+ * publication table.
+ */
if(!attgenlist[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))));
+
+ /*
+ * 'gencollist' records that this is a generated column
in
+ * the subscription table. Later, we use this
information to
+ * skip adding this column to the column list for COPY.
+ */
gencollist[attnum] = true;
}
}
