Re: Proposal: Filter irrelevant change before reassemble transactions during logical decoding

Hi Ajin.

Some review comments for patch v16-0001.

======
Commit message

1.
A hash cache of relation file locators is implemented to cache whether a
relation is filterable or not. This ensures that we only need to retrieve

~

/hash cache/hash table/

(AFAICT you called this a hash table elsewhere in all code comments)

~~~

2.
Additionally, filtering is paused for transactions containing WAL records
(INTERNAL_SNAPSHOT, COMMAND_ID, or INVALIDATION) that modify the historical
snapshot constructed during logical decoding. This pause is necessary because
constructing a correct historical snapshot for searching publication
information requires processing these WAL records.

~

IIUC, the "pause" here is really referring to the 100 changes
following an unfilterable txn. So, I don't think what you are
describing here is a "pause" -- it is just another reason for the tx
to be marked unfilterable, and the pause logic will take care of
itself. So, maybe it should be worded more like

SUGGESTION
Additionally, transactions containing WAL records (INTERNAL_SNAPSHOT,
COMMAND_ID, or INVALIDATION) that modify the historical
snapshot constructed during logical decoding are deemed unfilterable.
This is necessary because constructing a correct historical snapshot
for searching publication information requires processing these WAL
records.

======
src/backend/replication/logical/reorderbuffer.c

3. Header comment.

If you change the commit message based on previous suggestions, then
change the comment here also to match it.

~~~

4.
+/*
+ * After encountering a change that cannot be filtered out, filtering is
+ * temporarily suspended. Filtering resumes after processing every 100 changes.
+ * This strategy helps to minimize the overhead of performing a hash table
+ * search for each record, especially when most changes are not filterable.
+ */
+#define CHANGES_THRESHOLD_FOR_FILTER 100

/every 100/the next 100/

~~~

+ReorderBufferFilterByRelFileLocator:

5.
+ * if we are decoding a transaction without these records. See comment on top
+ * of GetDecodableRelation() to see list of relations that are not
+ * decoded by the reorderbuffer.

/to see list of relations/to see the kinds of relation/

======
Kind Regards,
Peter Smith.
Fujitsu Australia