Re: row filtering for logical replication

On Thu, Dec 2, 2021 at 6:18 PM Peter Smith <smithpb2250@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