On Thu, Dec 2, 2021 at 6:18 PM Peter Smith <smithpb2...@gmail.com> wrote: > > PSA a new v44* patch set. >
Some initial comments: 0001 src/backend/replication/logical/tablesync.c (1) In fetch_remote_table_info update, "list_free(*qual);" should be "list_free_deep(*qual);" doc/src/sgml/ref/create_subscription.sgml (2) Refer to Notes Perhaps a link to the Notes section should be used here, as follows: - copied. Refer to the Notes section below. + copied. Refer to the <xref linkend="sql-createsubscription-notes"/> section below. - <refsect1> + <refsect1 id="sql-createsubscription-notes" xreflabel="Notes"> 0002 1) Typo in patch comment "Specifially" src/backend/catalog/pg_publication.c 2) bms_replident comment Member "Bitmapset *bms_replident;" in rf_context should have a comment, maybe something like "set of replica identity col indexes". 3) errdetail message In rowfilter_walker(), the "forbidden" errdetail message is loaded using gettext() in one instance, but just a raw formatted string in other cases. Shouldn't they all consistently be translated strings? 0003 src/backend/replication/logical/proto.c 1) logicalrep_write_tuple (i) if (slot == NULL || TTS_EMPTY(slot)) can be replaced with: if (TupIsNull(slot)) (ii) In the above case (where values and nulls are palloc'd), shouldn't the values and nulls be pfree()d at the end of the function? 0005 src/backend/utils/cache/relcache.c (1) RelationGetInvalRowFilterCol Shouldn't "rfnode" be pfree()d after use? Regards, Greg Nancarrow Fujitsu Australia
