Here are my review comments for patch v34-0001
======
doc/src/sgml/ddl.sgml
1.
- Generated columns are skipped for logical replication and cannot be
- specified in a <command>CREATE PUBLICATION</command> column list.
+ Generated columns may be skipped during logical replication
according to the
+ <command>CREATE PUBLICATION</command> parameter
+ <link
linkend="sql-createpublication-params-with-publish-generated-columns">
+ <literal>publish_generated_columns</literal></link>.
This information is not quite correct because it makes no mention of
PUBLICATION column lists. OTOH I replaced all this paragraph in the
0003 patch anyhow, so maybe it is not worth worrying about this review
comment.
======
src/backend/catalog/pg_publication.c
2.
- if (TupleDescAttr(tupdesc, attnum - 1)->attgenerated)
- ereport(ERROR,
+ if (TupleDescAttr(tupdesc, attnum - 1)->attgenerated && !pubgencols)
+ ereport(WARNING,
errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- errmsg("cannot use generated column \"%s\" in publication column list",
+ errmsg("specified generated column \"%s\" in publication column list
when publish_generated_columns as false",
colname));
Back when I proposed to have this WARNING I don't think there existed
the idea that the PUBLICATION column list would override the
'publish_generated_columns' parameter. But now that this is the
implementation, I am no longer 100% sure if a warning should be given
at all because this can be a perfectly valid combination. What do
others think?
======
src/backend/replication/pgoutput/pgoutput.c
3. prepare_all_columns_bms
3a.
nit - minor rewording function comment
~
3b.
I am not sure this is a good function name, particularly since it does
not return "all" columns. Can you name it more for what it does?
~~~
4. pgoutput_column_list_init
/*
- * 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).
+ * To handle cases where the publish_generated_columns option isn't
+ * specified for all tables in a publication, the pubgencolumns check
+ * needs to be performed. In such cases, we must create a column list
+ * that excludes generated columns.
*/
- if (!pub->alltables)
+ if (!pub->alltables || !pub->pubgencols)
4a.
That comment still doesn't make much sense to me:
e.g.1. "To handle cases where the publish_generated_columns option
isn't specified for all tables in a publication". What is this trying
to say? A publication parameter is per-publication, so it is always
"for all tables in a publication".
e.g.2. " the pubgencolumns check" -- what is the pubgencols check?
Please rewrite the comment more clearly to explain the logic: What is
it doing? Why is it doing it?
~
4b.
+ if (!pub->alltables || !pub->pubgencols)
As mentioned in a previous review, there is too much negativity in
conditions like this. I think anything you can do to reverse all the
negativity will surely improve the readability of this function. See
[1 - #11a]
~
5.
- cols = pub_collist_to_bitmapset(cols, cfdatum,
- entry->entry_cxt);
+ if (!pub_rel_has_collist)
+ cols = pub_collist_to_bitmapset(cols, cfdatum, entry->entry_cxt);
+ else
+ cols = prepare_all_columns_bms(data, entry, desc);
Hm. Is that correct? The if/else is all backwards from what I would
have expected. IIUC the variable 'pub_rel_has_collist' is assigned to
the opposite value of what the name says, so then you are using it all
backwards to make it work.
nit - I have changed the code in the attachment how I think it should
be. Please check it makes sense.
~
6.
+ /* Get the number of live attributes. */
+ for (i = 0; i < desc->natts; i++)
nit - use a for-loop variable 'i'.
======
src/bin/pg_dump/pg_dump.c
7.
+ else if (fout->remoteVersion >= 130000)
+ appendPQExpBufferStr(query,
+ "SELECT p.tableoid, p.oid, p.pubname, "
+ "p.pubowner, "
+ "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, p.pubviaroot, false AS pubviagencols "
"FROM pg_publication p");
else if (fout->remoteVersion >= 110000)
appendPQExpBufferStr(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"p.pubowner, "
- "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, false AS pubviaroot "
+ "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
p.pubtruncate, false AS pubviaroot, false AS pubviagencols "
"FROM pg_publication p");
else
appendPQExpBufferStr(query,
"SELECT p.tableoid, p.oid, p.pubname, "
"p.pubowner, "
- "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, false AS
pubtruncate, false AS pubviaroot "
+ "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, false AS
pubtruncate, false AS pubviaroot, false AS pubviagencols "
"FROM pg_publication p");
These changes are all wrong due to a typo. There is no such column as
'pubviagencols'. I made these changes in the nit attachment. Please
check it for correctness.
s/pubviagencols/pubgencols/
======
src/test/regress/sql/publication.sql
8.
--- error: generated column "d" can't be in list
+-- ok: generated columns can be in the list too
nit - name the generated column "d", to clarify this comment
~~~
9.
+-- Test the publication 'publish_generated_columns' parameter enabled
or disabled for different combinations.
nit - add another "======" separator comment before the new test
nit - some minor adjustments to whitespace and comments
~~~
10.
+-- No gencols in column list with 'publish_generated_columns'=false.
+CREATE PUBLICATION pub3 WITH (publish_generated_columns=false);
+-- ALTER PUBLICATION to add gencols to column list.
+ALTER PUBLICATION pub3 ADD TABLE gencols(a, gen1);
nit - by adding and removing collist for the existing table, we don't
need to have a 'pub3'.
======
src/test/subscription/t/031_column_list.pl
11.
It is not clear to me -- is there, or is there not yet any test case
for the multiple publication issues that were discussed previously?
e.g. when the same table has gencols but there are multiple
publications for the same subscription and the
'publish_generated_columns' parameter or column lists conflict.
======
[1]
https://www.postgresql.org/message-id/CAHut%2BPt9ArtYc1Vtb1DqzEEFwjcoDHMyRCnUqkHQeFJuMCDkvQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/pgoutput/pgoutput.c
b/src/backend/replication/pgoutput/pgoutput.c
index 24c56ed..da7ebe9 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1009,8 +1009,9 @@ pgoutput_row_filter_init(PGOutputData *data, List
*publications,
}
/*
- * Prepare new column list bitmap.
- * This encompasses all table columns, excluding the generated ones.
+ * Return a column list bitmap for the specified table.
+ *
+ * Generated columns are excluded.
*/
static Bitmapset *
prepare_all_columns_bms(PGOutputData *data, RelationSyncEntry *entry,
@@ -1079,7 +1080,7 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
*/
if (!pub->alltables || !pub->pubgencols)
{
- bool pub_rel_has_collist = true;
+ bool pub_rel_has_collist = false;
/*
* Check for the presence of a column list in this
publication.
@@ -1094,28 +1095,31 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
if (HeapTupleIsValid(cftuple))
{
+ bool pub_no_list = true;
+
/* Lookup the column list attribute. */
cfdatum = SysCacheGetAttr(PUBLICATIONRELMAP,
cftuple,
Anum_pg_publication_rel_prattrs,
-
&pub_rel_has_collist);
+
&pub_no_list);
+
+ pub_rel_has_collist = !pub_no_list;
}
/* Build the column list bitmap in the per-entry
context. */
- if (!pub_rel_has_collist || !pub->pubgencols)
+ if (pub_rel_has_collist || !pub->pubgencols)
{
- int i;
int nliveatts = 0;
TupleDesc desc =
RelationGetDescr(relation);
pgoutput_ensure_entry_cxt(data, entry);
- if (!pub_rel_has_collist)
+ if (pub_rel_has_collist)
cols = pub_collist_to_bitmapset(cols,
cfdatum, entry->entry_cxt);
else
cols = prepare_all_columns_bms(data,
entry, desc);
/* Get the number of live attributes. */
- for (i = 0; i < desc->natts; i++)
+ for (int i = 0; i < desc->natts; i++)
{
Form_pg_attribute att =
TupleDescAttr(desc, i);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a0dad1e..ec12fd9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4304,19 +4304,19 @@ getPublications(Archive *fout)
appendPQExpBufferStr(query,
"SELECT p.tableoid,
p.oid, p.pubname, "
"p.pubowner, "
- "p.puballtables,
p.pubinsert, p.pubupdate, p.pubdelete, p.pubtruncate, p.pubviaroot, false AS
pubviagencols "
+ "p.puballtables,
p.pubinsert, p.pubupdate, p.pubdelete, p.pubtruncate, p.pubviaroot, false AS
pubgencols "
"FROM pg_publication
p");
else if (fout->remoteVersion >= 110000)
appendPQExpBufferStr(query,
"SELECT p.tableoid,
p.oid, p.pubname, "
"p.pubowner, "
- "p.puballtables,
p.pubinsert, p.pubupdate, p.pubdelete, p.pubtruncate, false AS pubviaroot,
false AS pubviagencols "
+ "p.puballtables,
p.pubinsert, p.pubupdate, p.pubdelete, p.pubtruncate, false AS pubviaroot,
false AS pubgencols "
"FROM pg_publication
p");
else
appendPQExpBufferStr(query,
"SELECT p.tableoid,
p.oid, p.pubname, "
"p.pubowner, "
- "p.puballtables,
p.pubinsert, p.pubupdate, p.pubdelete, false AS pubtruncate, false AS
pubviaroot, false AS pubviagencols "
+ "p.puballtables,
p.pubinsert, p.pubupdate, p.pubdelete, false AS pubtruncate, false AS
pubviaroot, false AS pubgencols "
"FROM pg_publication
p");
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
diff --git a/src/test/regress/expected/publication.out
b/src/test/regress/expected/publication.out
index a26b5a4..a532cd6 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -693,7 +693,7 @@ UPDATE testpub_tbl5 SET a = 1;
ERROR: cannot update table "testpub_tbl5"
DETAIL: Column list used by the publication does not cover the replica
identity.
ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
--- ok: generated columns can be in the list too
+-- ok: generated column "d" can be in the list too
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, d);
WARNING: specified generated column "d" in publication column list when
publish_generated_columns as false
ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
@@ -1756,6 +1756,7 @@ DROP PUBLICATION pub;
DROP TABLE sch1.tbl1;
DROP SCHEMA sch1 cascade;
DROP SCHEMA sch2 cascade;
+-- ======================================================
-- Test the publication 'publish_generated_columns' parameter enabled or
disabled
SET client_min_messages = 'ERROR';
CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_generated_columns=1);
@@ -1777,30 +1778,29 @@ CREATE PUBLICATION pub2 FOR ALL TABLES WITH
(publish_generated_columns=0);
RESET client_min_messages;
DROP PUBLICATION pub1;
DROP PUBLICATION pub2;
--- Test the publication 'publish_generated_columns' parameter enabled or
disabled for different combinations.
+-- Test the 'publish_generated_columns' parameter enabled or disabled for
+-- different scenarios with/without generated columns in column lists.
SET client_min_messages = 'WARNING';
CREATE TABLE gencols (a int, gen1 int GENERATED ALWAYS AS (a * 2) STORED);
--- gencols in column list with 'publish_generated_columns'=false.
+-- gencols in column list with 'publish_generated_columns'=false
CREATE PUBLICATION pub1 FOR table gencols(a, gen1) WITH
(publish_generated_columns=false);
WARNING: specified generated column "gen1" in publication column list when
publish_generated_columns as false
WARNING: "wal_level" is insufficient to publish logical changes
HINT: Set "wal_level" to "logical" before creating subscriptions.
--- gencols in column list with 'publish_generated_columns'=true.
+-- gencols in column list with 'publish_generated_columns'=true
CREATE PUBLICATION pub2 FOR table gencols(a, gen1) WITH
(publish_generated_columns=true);
WARNING: "wal_level" is insufficient to publish logical changes
HINT: Set "wal_level" to "logical" before creating subscriptions.
--- ALTER PUBLICATION setting 'publication_generate_columns'=false.
+-- gencols in column list, then set 'publication_generate_columns'=false
ALTER PUBLICATION pub2 SET (publish_generated_columns = false);
--- No gencols in column list with 'publish_generated_columns'=false.
-CREATE PUBLICATION pub3 WITH (publish_generated_columns=false);
-WARNING: "wal_level" is insufficient to publish logical changes
-HINT: Set "wal_level" to "logical" before creating subscriptions.
--- ALTER PUBLICATION to add gencols to column list.
-ALTER PUBLICATION pub3 ADD TABLE gencols(a, gen1);
+-- remove gencols from column list, when 'publish_generated_columns'=false
+ALTER PUBLICATION pub2 SET TABLE gencols(a);
+-- Add gencols in column list, when 'publish_generated_columns'=false.
+ALTER PUBLICATION pub2 SET TABLE gencols(a, gen1);
+WARNING: specified generated column "gen1" in publication column list when
publish_generated_columns as false
WARNING: specified generated column "gen1" in publication column list when
publish_generated_columns as false
DROP PUBLICATION pub1;
DROP PUBLICATION pub2;
-DROP PUBLICATION pub3;
DROP TABLE gencols;
RESET client_min_messages;
RESET SESSION AUTHORIZATION;
diff --git a/src/test/regress/sql/publication.sql
b/src/test/regress/sql/publication.sql
index d7b43dc..6a74fd6 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -415,7 +415,7 @@ ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5
(a, x);
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (b, c);
UPDATE testpub_tbl5 SET a = 1;
ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
--- ok: generated columns can be in the list too
+-- ok: generated column "d" can be in the list too
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, d);
ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
-- error: system attributes "ctid" not allowed in column list
@@ -1112,6 +1112,7 @@ DROP PUBLICATION pub;
DROP TABLE sch1.tbl1;
DROP SCHEMA sch1 cascade;
DROP SCHEMA sch2 cascade;
+-- ======================================================
-- Test the publication 'publish_generated_columns' parameter enabled or
disabled
SET client_min_messages = 'ERROR';
@@ -1125,24 +1126,28 @@ RESET client_min_messages;
DROP PUBLICATION pub1;
DROP PUBLICATION pub2;
--- Test the publication 'publish_generated_columns' parameter enabled or
disabled for different combinations.
+-- Test the 'publish_generated_columns' parameter enabled or disabled for
+-- different scenarios with/without generated columns in column lists.
SET client_min_messages = 'WARNING';
CREATE TABLE gencols (a int, gen1 int GENERATED ALWAYS AS (a * 2) STORED);
--- gencols in column list with 'publish_generated_columns'=false.
+
+-- gencols in column list with 'publish_generated_columns'=false
CREATE PUBLICATION pub1 FOR table gencols(a, gen1) WITH
(publish_generated_columns=false);
--- gencols in column list with 'publish_generated_columns'=true.
+
+-- gencols in column list with 'publish_generated_columns'=true
CREATE PUBLICATION pub2 FOR table gencols(a, gen1) WITH
(publish_generated_columns=true);
--- ALTER PUBLICATION setting 'publication_generate_columns'=false.
+
+-- gencols in column list, then set 'publication_generate_columns'=false
ALTER PUBLICATION pub2 SET (publish_generated_columns = false);
--- No gencols in column list with 'publish_generated_columns'=false.
-CREATE PUBLICATION pub3 WITH (publish_generated_columns=false);
--- ALTER PUBLICATION to add gencols to column list.
-ALTER PUBLICATION pub3 ADD TABLE gencols(a, gen1);
+-- remove gencols from column list, when 'publish_generated_columns'=false
+ALTER PUBLICATION pub2 SET TABLE gencols(a);
+
+-- Add gencols in column list, when 'publish_generated_columns'=false.
+ALTER PUBLICATION pub2 SET TABLE gencols(a, gen1);
DROP PUBLICATION pub1;
DROP PUBLICATION pub2;
-DROP PUBLICATION pub3;
DROP TABLE gencols;
RESET client_min_messages;
