On Wed, Dec 15, 2021 at 3:50 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Mon, Dec 13, 2021 at 8:49 PM Peter Smith <smithpb2...@gmail.com> wrote: > > > > PSA the v46* patch set. > > > > 0001 > ...
> (2) In the 0001 patch comment, the term "publication filter" is used > in one place, and in others "row filter" or "row-filter". > Fixed in v47 [1] > > src/backend/catalog/pg_publication.c > (3) GetTransformedWhereClause() is missing a function comment. > Fixed in v47 [1] > (4) > The following comment seems incomplete: > > + /* Fix up collation information */ > + whereclause = GetTransformedWhereClause(pstate, pri, true); > > Fixed in v47 [1] > src/backend/parser/parse_relation.c > (5) > wording? consistent? > Shouldn't it be "publication WHERE expression" for consistency? > In v47 [1] this message is removed when the KIND is removed. > + errmsg("publication row-filter WHERE invalid reference to table \"%s\"", > + relation->relname), > > > src/backend/replication/logical/tablesync.c > (6) > > (i) Improve wording: > > BEFORE: > /* > * Get information about remote relation in similar fashion the RELATION > - * message provides during replication. > + * message provides during replication. This function also returns the > relation > + * qualifications to be used in COPY command. > */ > > AFTER: > /* > - * Get information about remote relation in similar fashion the RELATION > - * message provides during replication. > + * Get information about a remote relation, in a similar fashion to > how the RELATION > + * message provides information during replication. This function > also returns the relation > + * qualifications to be used in the COPY command. > */ > Fixed in v47 [1] > (ii) fetch_remote_table_info() doesn't currently account for ALL > TABLES and ALL TABLES IN SCHEMA. > > Fixed in v47 [1] ... > > > 0002 > > src/backend/catalog/pg_publication.c > (1) rowfilter_walker() > One of the errdetail messages doesn't begin with an uppercase letter: > > + errdetail_msg = _("user-defined types are not allowed"); > > Fixed in v47 [1] > src/backend/executor/execReplication.c > (2) CheckCmdReplicaIdentity() > > Strictly speaking, the following: > > + if (invalid_rfcolnum) > > should be: > > + if (invalid_rfcolnum != InvalidAttrNumber) > > Fixed in v47 [1] > 0003 > > src/backend/replication/logical/tablesync.c > (1) > Column name in comment should be "puballtables" not "puballtable": > > + * If any publication has puballtable true then all row-filtering is > Fixed in v47 [1] > (2) pgoutput_row_filter_init() > > There should be a space before the final "*/" (so the asterisks align). > Also, should say "... treated the same". > > /* > + * If the publication is FOR ALL TABLES then it is treated same as if this > + * table has no filters (even if for some other publication it does). > + */ > > Fixed in v47 [1] ------ [1] https://www.postgresql.org/message-id/CAHut%2BPtjsj_OVMWEdYp2Wq19%3DH5D4Vgta43FbFVDYr2LuS_djg%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia