Here are review comments for v15-0001
======
doc/src/sgml/ddl.sgml
nitpick - there was a comma (,) which should be a period (.)
======
.../libpqwalreceiver/libpqwalreceiver.c
1.
+ if (options->proto.logical.include_generated_columns &&
+ PQserverVersion(conn->streamConn) >= 170000)
+ appendStringInfoString(&cmd, ", include_generated_columns 'true'");
+
Should now say >= 180000
======
src/backend/replication/pgoutput/pgoutput.c
nitpick - comment wording for RelationSyncEntry.collist.
~~
2.
pgoutput_column_list_init:
I found the current logic to be quite confusing. I assume the code is
working OK, because AFAIK there are plenty of tests and they are all
passing, but the logic seems somewhat repetitive and there are also no
comments to explain it adding to my confusion.
IIUC, PRIOR TO THIS PATCH:
BMS field 'columns' represented the "columns of the column list" or it
was NULL if there was no publication column list (and it was also NULL
if the column list contained every column).
IIUC NOW, WITH THIS PATCH:
The BMS field 'columns' meaning is changed slightly to be something
like "columns to be replicated" or NULL if all columns are to be
replicated. This is almost the same thing except we are now handing
the generated columns up-front, so generated columns will or won't
appear in the BMS according to the "include_generated_columns"
parameter. See how this is all a bit subtle which is why copious new
comments are required to explain it...
So, although the test result evidence suggests this is working OK, I
have many questions/issues about it. Here are some to start with:
2a. It needs a lot more (summary and detailed) comments explaining the
logic now that the meaning is slightly different.
2b. What is the story with the FOR ALL TABLES case now? Previously,
there would always be NULL 'columns' for "FOR ALL TABLES" case -- the
comment still says so. But now you've tacked on a 2nd pass of
iterations to build the BMS outside of the "if (!pub->alltables)"
check. Is that OK?
2c. The following logic seemed unexpected:
- if (bms_num_members(cols) == nliveatts)
+ if (bms_num_members(cols) == nliveatts &&
+ data->include_generated_columns)
{
bms_free(cols);
cols = NULL;
`
I had thought the above code would look different -- more like:
if (att->attgenerated && !data->include_generated_columns)
continue;
nliveatts++;
...
2d. Was so much duplicated code necessary? It feels like the whole
"Get the number of live attributes." and assignment of cols to NULL
might be made common to both code paths.
2e. I'm beginning to question the pros/cons of the new BMS logic; I
had suggested trying this way (processing the generated columns
up-front in the BMS 'columns' list) to reduce patch code and simplify
all the subsequent API delegation of "include_generated_cloumns"
everywhere like it was in v14-0001. Indeed, that part was a success
and the patch is now smaller. But I don't like much that we've traded
reduced code overall for increased confusing code in that BMS
function. If all this BMS code can be refactored and commented to be
easier to understand then maybe all will be well, but if it can't then
maybe this BMS change was a bridge too far. I haven't given up on it
just yet, but I wonder what was your opinion about it, and do other
people have thoughts about whether this was the good direction to
take?
======
src/bin/pg_dump/pg_dump.c
3.
+ if (fout->remoteVersion >= 170000)
+ appendPQExpBufferStr(query,
+ " s.subincludegencols\n");
+ else
+ appendPQExpBufferStr(query,
+ " false AS subincludegencols\n");
Should now say >= 180000
======
src/bin/psql/describe.c
4.
+ /* include_generated_columns is only supported in v18 and higher */
+ if (pset.sversion >= 170000)
+ appendPQExpBuffer(&buf,
+ ", subincludegencols AS \"%s\"\n",
+ gettext_noop("Include generated columns"));
+
Should now say >= 180000
======
src/include/catalog/pg_subscription.h
nitpick - let's make the comment the same as in WalRcvStreamOptions
======
src/include/replication/logicalproto.h
nitpick - extern for logicalrep_write_update should be unchanged by this patch
======
src/test/regress/sql/subscription.sql
nitpick = the comment "include_generated_columns and copy_data = true
are mutually exclusive" is not necessary because this all falls under
the existing comment "fail - invalid option combinations"
nitpick - let's explicitly put "copy_data = true" in the CREATE
SUBSCRIPTION to make it more obvious
======
99. Please also refer to the attached 'diffs' patch which implements
all of my nitpicks issues mentioned above.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index a296305..f7c57d4 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -517,7 +517,7 @@ CREATE TABLE people (
Generated columns may be skipped during logical replication according to
the
<command>CREATE SUBSCRIPTION</command> option
<link
linkend="sql-createsubscription-params-with-include-generated-columns">
- <literal>include_generated_columns</literal></link>,
+ <literal>include_generated_columns</literal></link>.
</para>
</listitem>
</itemizedlist>
diff --git a/src/backend/replication/pgoutput/pgoutput.c
b/src/backend/replication/pgoutput/pgoutput.c
index 5ff5078..52f1551 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -164,7 +164,7 @@ typedef struct RelationSyncEntry
AttrMap *attrmap;
/*
- * Columns should be publicated, or NULL if all columns are included
+ * Columns to be published, or NULL if all columns are included
* implicitly. This bitmap only considers the column list of the
* publication and include_generated_columns option: other reasons
should
* be checked at user side. Note that the attnums in this bitmap are
not
diff --git a/src/include/catalog/pg_subscription.h
b/src/include/catalog/pg_subscription.h
index 0bb5782..50c5911 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -160,7 +160,7 @@ typedef struct Subscription
List *publications; /* List of publication names to
subscribe to */
char *origin; /* Only publish data
originating from the
* specified
origin */
- bool includegencols; /* Publish generated columns data */
+ bool includegencols; /* Publish generated columns */
} Subscription;
/* Disallow streaming in-progress transactions. */
diff --git a/src/include/replication/logicalproto.h
b/src/include/replication/logicalproto.h
index b9a64d9..c409638 100644
--- a/src/include/replication/logicalproto.h
+++ b/src/include/replication/logicalproto.h
@@ -230,8 +230,7 @@ extern LogicalRepRelId logicalrep_read_insert(StringInfo
in, LogicalRepTupleData
extern void logicalrep_write_update(StringInfo out, TransactionId xid,
Relation rel,
TupleTableSlot *oldslot,
-
TupleTableSlot *newslot, bool binary,
-
Bitmapset *columns);
+
TupleTableSlot *newslot, bool binary, Bitmapset *columns);
extern LogicalRepRelId logicalrep_read_update(StringInfo in,
bool *has_oldtuple, LogicalRepTupleData *oldtup,
LogicalRepTupleData *newtup);
diff --git a/src/test/regress/expected/subscription.out
b/src/test/regress/expected/subscription.out
index 36916c0..1a99099 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -99,8 +99,7 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION
'dbname=regress_doesnotexist' PU
ERROR: subscription with slot_name = NONE must also set create_slot = false
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
ERROR: subscription with slot_name = NONE must also set enabled = false
--- fail - include_generated_columns and copy_data = true are mutually exclusive
-CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (include_generated_columns = true);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (include_generated_columns = true, copy_data = true);
ERROR: copy_data = true and include_generated_columns = true are mutually
exclusive options
-- fail - include_generated_columns must be boolean
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false, include_generated_columns = foo);
diff --git a/src/test/regress/sql/subscription.sql
b/src/test/regress/sql/subscription.sql
index 7944152..7922dfd 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -59,9 +59,7 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION
'dbname=regress_doesnotexist' PU
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE);
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
-
--- fail - include_generated_columns and copy_data = true are mutually exclusive
-CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (include_generated_columns = true);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (include_generated_columns = true, copy_data = true);
-- fail - include_generated_columns must be boolean
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false, include_generated_columns = foo);
