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

Hi Ajin,

Some review comments for patch v16-0002.

======
doc/src/sgml/logicaldecoding.sgml

1.
+ To indicate that decoding can be skipped for the given change
+ <parameter>change_type</parameter>, return <literal>true</literal>;
+ <literal>false</literal> otherwise.

/change <parameter>change_type</parameter>/<parameter>change_type</parameter>/

(don't need to say "change" twice)

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

SkipThisChange:

2.
+/* Function to determine whether to skip the change */
+static bool SkipThisChange(LogicalDecodingContext *ctx, XLogRecPtr
origptr, TransactionId xid,
+ RelFileLocator *target_locator, ReorderBufferChangeType change_type)
+{
+ return (FilterChangeIsEnabled(ctx) &&
+ ReorderBufferFilterByRelFileLocator(ctx->reorder, xid, origptr,
target_locator,
+ change_type));
+}

2a.
By convention the function return should be on its own line.

~

2b.
Probably this should be declared *inline* like other functions similar to this?

~

2c.
Consider renaming this function to FilterChange(...). I think that
might align better with the other functions like
FilterChangeIsEnabled, FilterByOrigin etc which all refer to
"filtering" instead of "skipping".

~~~

3.
+ /*
+ * When filtering changes, determine if the relation associated with the change
+ * can be skipped. This could be because the relation is unlogged or because
+ * the plugin has opted to exclude this relation from decoding.
+ */
+ if (SkipThisChange(ctx, buf->origptr, XLogRecGetXid(r), &target_locator,
+ REORDER_BUFFER_CHANGE_INSERT))

All these block comments prior to the calls to SkipThisChange seem
slightly overkill (I think Shlok also reports this). IMO this comment
can be much simpler:
e.g.
Can the relation associated with this change be skipped?

This is repeated in multiple places: DecodeInsert, DecodeUpdate,
DecodeDelete, DecodeMultiInsert, DecodeSpecConfirm

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

filter_change_cb_wrapper:

4.
+ /* check if the filter change callback is supported */
+ if (ctx->callbacks.filter_change_cb == NULL)
+ return false;
+

Other optional callbacks are more terse and just say like below, with
no comment.
if (!ctx->callbacks.truncate_cb)
return;
SUGGESTION (do the same here)
if (!ctx->callbacks.filter_change_cb)
return false;

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

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

I agree with Shlok. This change seems to belong in patch 0001.

~~~

ReorderBufferFilterByRelFileLocator

6.
ReorderBufferFilterByRelFileLocator(ReorderBuffer *rb, TransactionId xid,
XLogRecPtr lsn, RelFileLocator *rlocator,
ReorderBufferChangeType change_type)
~

The function comment for this says "Returns true if the relation can
be filtered; otherwise, false.". I'm not sure that it is strictly
correct. IMO the comment should explain more about how the return
boolean depends also on the *change_type*. e.g. IIUC you could be able
to filter INSERTS but not filter DELETES even for the same relation.

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