Re: Column Filtering in Logical Replication

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