Hi Peter,
Thanks for the review and sorry for taking so long.
I've addressed most of the comments in the patch I sent a couple minutes
ago. More comments in-line:
On 1/28/22 09:39, Peter Smith wrote:
Here are some review comments for the v17-0001 patch.
~~~
1. Commit message
If no column list is specified, all the columns are replicated, as
previously
Missing period (.) at the end of that sentence.
I plan to reword that anyway.
~~~
2. doc/src/sgml/catalogs.sgml
+ <para>
+ This is an array of values that indicates which table columns are
+ part of the publication. For example a value of <literal>1 3</literal>
+ would mean that the first and the third table columns are published.
+ A null value indicates that all attributes are published.
+ </para></entry>
Missing comma:
"For example" --> "For example,"
Fixed.
Terms:
The text seems to jump between "columns" and "attributes". Perhaps,
for consistency, that last sentence should say: "A null value
indicates that all columns are published."
Yeah, but that's a pre-existing problem. I've modified the parts added
by the patch to use "columns" though.
~~~
3. doc/src/sgml/protocol.sgml
</variablelist>
- Next, the following message part appears for each column
(except generated columns):
+ Next, the following message part appears for each column (except
+ generated columns and other columns that don't appear in the column
+ filter list, for tables that have one):
<variablelist>
Perhaps that can be expressed more simply, like:
Next, the following message part appears for each column (except
generated columns and other columns not present in the optional column
filter list):
Not sure. I'll think about it.
~~~
4. doc/src/sgml/ref/alter_publication.sgml
+ALTER PUBLICATION <replaceable class="parameter">name</replaceable>
ALTER TABLE <replaceable
class="parameter">publication_object</replaceable> SET COLUMNS { (
<replaceable class="parameter">name</replaceable> [, ...] ) | ALL }
The syntax chart looks strange because there is already a "TABLE" and
a column_name list within the "publication_object" definition, so do
ALTER TABLE and publication_object co-exist?
According to the current documentation it suggests nonsense like below is valid:
ALTER PUBLICATION mypublication ALTER TABLE TABLE t1 (a,b,c) SET
COLUMNS (a,b,c);
Yeah, I think that's wrong. I think "publication_object" is wrong in
this place, so I've used "table_name".
--
But more fundamentally, I don't see why any new syntax is even needed at all.
Instead of:
ALTER PUBLICATION mypublication ALTER TABLE users SET COLUMNS
(user_id, firstname, lastname);
Why not just:
ALTER PUBLICATION mypublication ALTER TABLE users (user_id, firstname,
lastname);
I haven't modified the grammar yet, but I agree SET COLUMNS seems a bit
unnecessary. It also seems a bit inconsistent with ADD TABLE which
simply lists the columns right adter the table name.
Then, if the altered table defines a *different* column list then it
would be functionally equivalent to whatever your SET COLUMNS is doing
now. AFAIK this is how the Row-Filter [1] works, so that altering an
existing table to have a different Row-Filter just overwrites that
table's filter. IMO the Col-Filter behaviour should work the same as
that - "SET COLUMNS" is redundant.
I'm sorry, I don't understand what this is saying :-(
~~~
5. doc/src/sgml/ref/alter_publication.sgml
- TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [, ... ]
+ TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [ ( <replaceable
class="parameter">column_name</replaceable>, [, ... ] ) ] [, ... ]
That extra comma after the "column_name" seems wrong because there is
one already in "[, ... ]".
Fixed.
~~~
6. doc/src/sgml/ref/create_publication.sgml
- TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [, ... ]
+ TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [ ( <replaceable
class="parameter">column_name</replaceable>, [, ... ] ) ] [, ... ]
(Same as comment #5).
That extra comma after the "column_name" seems wrong because there is
one already in "[, ... ]".
Fixed.
~~~
7. doc/src/sgml/ref/create_publication.sgml
+ <para>
+ When a column list is specified, only the listed columns are replicated;
+ any other columns are ignored for the purpose of replication through
+ this publication. If no column list is specified, all columns of the
+ table are replicated through this publication, including any columns
+ added later. If a column list is specified, it must include the replica
+ identity columns.
+ </para>
Suggest to re-word this a bit simpler:
e.g.
- "listed columns" --> "named columns"
- I don't think it is necessary to say the unlisted columns are ignored.
- I didn't think it is necessary to say "though this publication"
AFTER
When a column list is specified, only the named columns are replicated.
If no column list is specified, all columns of the table are replicated,
including any columns added later. If a column list is specified, it must
include the replica identity columns.
Fixed, seems reasonable.
~~~
8. doc/src/sgml/ref/create_publication.sgml
Consider adding another example showing a CREATE PUBLICATION which has
a column list.
Added.
~~~
9. src/backend/catalog/pg_publication.c - check_publication_add_relation
/*
- * Check if relation can be in given publication and throws appropriate
- * error if not.
+ * Check if relation can be in given publication and that the column
+ * filter is sensible, and throws appropriate error if not.
+ *
+ * targetcols is the bitmapset of attribute numbers given in the column list,
+ * or NULL if it was not specified.
*/
Typo: "targetcols" --> "columns" ??
Right, I noticed that too.
~~~
10. src/backend/catalog/pg_publication.c - check_publication_add_relation
+
+ /* Make sure the column list checks out */
+ if (columns != NULL)
+ {
Perhaps "checks out" could be worded better.
Right, I expanded that in my review.
~~~
11. src/backend/catalog/pg_publication.c - check_publication_add_relation
+ /* Make sure the column list checks out */
+ if (columns != NULL)
+ {
+ /*
+ * Even if the user listed all columns in the column list, we cannot
+ * allow a column list to be specified when REPLICA IDENTITY is FULL;
+ * that would cause problems if a new column is added later, because
+ * the new column would have to be included (because of being part of
+ * the replica identity) but it's technically not allowed (because of
+ * not being in the publication's column list yet). So reject this
+ * case altogether.
+ */
+ if (replidentfull)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("invalid column list for publishing relation \"%s\"",
+ RelationGetRelationName(targetrel)),
+ errdetail("Cannot specify a column list on relations with REPLICA
IDENTITY FULL."));
+
+ check_publication_columns(pub, targetrel, columns);
+ }
IIUC almost all of the above comment and code is redundant because by
calling the check_publication_columns function it will do exactly the
same check...
So, that entire slab might be replaced by 2 lines:
if (columns != NULL)
check_publication_columns(pub, targetrel, columns);
You're right. But I think we can make that even simpler by moving even
the (columns!=NULL) check into the function.
~~~
12. src/backend/catalog/pg_publication.c - publication_set_table_columns
+publication_set_table_columns(Relation pubrel, HeapTuple pubreltup,
+ Relation targetrel, List *columns)
+{
+ Bitmapset *attset;
+ AttrNumber *attarray;
+ HeapTuple copytup;
+ int natts;
+ bool nulls[Natts_pg_publication_rel];
+ bool replaces[Natts_pg_publication_rel];
+ Datum values[Natts_pg_publication_rel];
+
+ memset(values, 0, sizeof(values));
+ memset(nulls, 0, sizeof(nulls));
+ memset(replaces, false, sizeof(replaces));
It seemed curious to use memset false for "replaces" but memset 0 for
"nulls", since they are both bool arrays (??)
Fixed.
~~~
13. src/backend/catalog/pg_publication.c - compare_int16
+/* qsort comparator for attnums */
+static int
+compare_int16(const void *a, const void *b)
+{
+ int av = *(const int16 *) a;
+ int bv = *(const int16 *) b;
+
+ /* this can't overflow if int is wider than int16 */
+ return (av - bv);
+}
This comparator seems common with another one already in the PG
source. Perhaps it would be better for generic comparators (like this
one) to be in some common code instead of scattered cut/paste copies
of the same thing.
I thought about it, but it doesn't really seem worth the effort.
~~~
14. src/backend/commands/publicationcmds.c - AlterPublicationTables
+ else if (stmt->action == AP_SetColumns)
+ {
+ Assert(schemaidlist == NIL);
+ Assert(list_length(tables) == 1);
+
+ PublicationSetColumns(stmt, pubform,
+ linitial_node(PublicationTable, tables));
+ }
(Same as my earlier review comment #4)
Suggest to call this PublicationSetColumns based on some smarter
detection logic of a changed column list. Please refer to the
Row-Filter patch [1] for this same function.
I don't understand. Comment #4 is about syntax, no?
~~~
15. src/backend/commands/publicationcmds.c - AlterPublicationTables
+ /* This is not needed to delete a table */
+ pubrel->columns = NIL;
Perhaps a more explanatory comment would be better there?
If I understand the comment, it says we don't actually need to set
columns to NIL. In which case we can just get rid of the change.
~~~
16. src/backend/commands/tablecmds.c - relation_mark_replica_identity
@@ -15841,6 +15871,7 @@ relation_mark_replica_identity(Relation rel,
char ri_type, Oid indexOid,
CatalogTupleUpdate(pg_index, &pg_index_tuple->t_self, pg_index_tuple);
InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
InvalidOid, is_internal);
+
/*
* Invalidate the relcache for the table, so that after we commit
* all sessions will refresh the table's replica identity index
Spurious whitespace change seemed unrelated to the Col-Filter patch.
Fixed.
~~~
17. src/backend/parser/gram.y
*
+ * ALTER PUBLICATION name SET COLUMNS table_name (column[, ...])
+ * ALTER PUBLICATION name SET COLUMNS table_name ALL
+ *
(Same as my earlier review comment #4)
IMO there was no need for the new syntax of SET COLUMNS.
Not modified yet, we'll see about the syntax.
~~~
18. src/backend/replication/logical/proto.c - logicalrep_write_attrs
- /* send number of live attributes */
- for (i = 0; i < desc->natts; i++)
- {
- if (TupleDescAttr(desc, i)->attisdropped || TupleDescAttr(desc,
i)->attgenerated)
- continue;
- nliveatts++;
- }
- pq_sendint16(out, nliveatts);
-
/* fetch bitmap of REPLICATION IDENTITY attributes */
replidentfull = (rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL);
if (!replidentfull)
idattrs = RelationGetIdentityKeyBitmap(rel);
+ /* send number of live attributes */
+ for (i = 0; i < desc->natts; i++)
+ {
+ Form_pg_attribute att = TupleDescAttr(desc, i);
+
+ if (att->attisdropped || att->attgenerated)
+ continue;
+ if (columns != NULL && !bms_is_member(att->attnum, columns))
+ continue;
+ nliveatts++;
+ }
+ pq_sendint16(out, nliveatts);
+
This change seemed to have the effect of moving that 4 lines of
"replidentfull" code from below the loop to above the loop. But moving
that code seems unrelated to the Col-Filter patch. (??).
Right, restored the original code.
~~~
19. src/backend/replication/logical/tablesync.c - fetch_remote_table_info
@@ -793,12 +877,12 @@ fetch_remote_table_info(char *nspname, char *relname,
ExecClearTuple(slot);
}
+
ExecDropSingleTupleTableSlot(slot);
-
- lrel->natts = natt;
-
walrcv_clear_result(res);
pfree(cmd.data);
+
+ lrel->natts = natt;
}
The shuffling of those few lines seems unrelated to any requirement of
the Col-Filter patch (??)
Yep, undone. I'd bet this is simply due to older versions of the patch
touching this place, and then undoing some of it.
~~~
20. src/backend/replication/logical/tablesync.c - copy_table
+ for (int i = 0; i < lrel.natts; i++)
+ {
+ appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
+ if (i < lrel.natts - 1)
+ appendStringInfoString(&cmd, ", ");
+ }
Perhaps that could be expressed more simply if the other way around like:
for (int i = 0; i < lrel.natts; i++)
{
if (i)
appendStringInfoString(&cmd, ", ");
appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
}
I used a slightly different version.
~~~
21. src/backend/replication/pgoutput/pgoutput.c
+
+ /*
+ * Set of columns included in the publication, or NULL if all columns are
+ * included implicitly. Note that the attnums in this list are not
+ * shifted by FirstLowInvalidHeapAttributeNumber.
+ */
+ Bitmapset *columns;
Typo: "in this list" --> "in this set" (??)
"bitmap" is what we call Bitmapset so I used that.
~~~
22. src/backend/replication/pgoutput/pgoutput.c - get_rel_sync_entry
* Don't publish changes for partitioned tables, because
- * publishing those of its partitions suffices, unless partition
- * changes won't be published due to pubviaroot being set.
+ * publishing those of its partitions suffices. (However, ignore
+ * this if partition changes are not to published due to
+ * pubviaroot being set.)
*/
This change seems unrelated to the Col-Filter patch, so perhaps it
should not be here at all.
Also, typo: "are not to published"
Yeah, unrelated. Reverted.
~~~
23. src/backend/replication/pgoutput/pgoutput.c - get_rel_sync_entry
+ /*
+ * Obtain columns published by this publication, and add them
+ * to the list for this rel. Note that if at least one
+ * publication has a empty column list, that means to publish
+ * everything; so if we saw a publication that includes all
+ * columns, skip this.
+ */
Typo: "a empty" --> "an empty"
Fixed.
~~~
24. src/backend/replication/pgoutput/pgoutput.c - get_rel_sync_entry
+ if (isnull)
+ {
+ /*
+ * If we see a publication with no columns, reset the
+ * list and ignore further ones.
+ */
Perhaps that comment is meant to say "with no column filter" instead
of "with no columns"?
Yep, fixed.
~~~
25. src/backend/replication/pgoutput/pgoutput.c - get_rel_sync_entry
+ if (isnull)
+ {
...
+ }
+ else if (!isnull)
+ {
...
+ }
Is the "if (!isnull)" in the else just to be really REALLY sure it is not null?
Double-tap ;-) Removed the condition.
~~~
26. src/bin/pg_dump/pg_dump.c - getPublicationTables
+ pubrinfo[i].pubrattrs = attribs->data;
+ }
+ else
+ pubrinfo[j].pubrattrs = NULL;
I got confused reading this code. Are those different indices 'i' and
'j' correct?
Good catch! I think you're right and it should be "j" in both places.
This'd only cause trouble in selective pg_dumps (when dumping selected
tables). The patch clearly needs some pg_dump tests.
~~~
27. src/bin/psql/describe.c
The Row-Filter [1] displays filter information not only for the psql
\dRp+ command but also for the psql \d <tablename> command. Perhaps
the Col-Filter patch should do that too.
Not sure.
~~~
28. src/bin/psql/tab-complete.c
@@ -1657,6 +1657,8 @@ psql_completion(const char *text, int start, int end)
/* ALTER PUBLICATION <name> ADD */
else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD"))
COMPLETE_WITH("ALL TABLES IN SCHEMA", "TABLE");
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "ADD", "TABLE"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, NULL);
/* ALTER PUBLICATION <name> DROP */
I am not sure about this one- is that change even related to the
Col-Filter patch or is this some unrelated bugfix?
Yeah, seems unrelated - possibly from a rebase or something. Removed.
~~~
29. src/include/catalog/pg_publication.h
@@ -86,6 +86,7 @@ typedef struct Publication
typedef struct PublicationRelInfo
{
Relation relation;
+ List *columns;
} PublicationRelInfo;
Perhaps that needs some comment. e.g. do you need to mention that a
NIL List means all columns?
I added a short comment.
~~~
30. src/include/nodes/parsenodes.h
@@ -3642,6 +3642,7 @@ typedef struct PublicationTable
{
NodeTag type;
RangeVar *relation; /* relation to be published */
+ List *columns; /* List of columns in a publication table */
} PublicationTable;
That comment "List of columns in a publication table" doesn't really
say anything helpful.
Perhaps it should mention that a NIL List means all table columns?
Not sure, seems fine.
~~~
31. src/test/regress/sql/publication.sql
The regression test file has an uncommon mixture of /* */ and -- style comments.
Perhaps change all the /* */ ones?
Yeah, that needs some cleanup. I haven't done anything about it yet.
~~~
32. src/test/regress/sql/publication.sql
+CREATE TABLE testpub_tbl5 (a int PRIMARY KEY, b text, c text,
+ d int generated always as (a + length(b)) stored);
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, x); -- error
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (b, c); -- error
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, d); -- error
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c); -- ok
For all these tests (and more) there seems not sufficient explanation
comments to say exactly what each test case is testing, e.g. *why* is
an "error" expected for some cases but "ok" for others.
Not sure. I think the error is generally obvious in the expected output.
~~~
33. src/test/regress/sql/publication.sql
"-- no dice"
(??) confusing comment.
Same as for the errors.
~~~
34. src/test/subscription/t/028_column_list.pl
I think a few more comments in this TAP file would help to make the
purpose of the tests more clear.
Yeah, the 0004 patch I shared a couple minutes ago does exactly that.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
