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

On Tue, 28 Jan 2025 at 08:40, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> Here's a patch-set created based on the design changes proposed by Hou-san.

Few comments:
1) Shouldn't we do the same thing for other DecodeXXX functions?
@@ -915,6 +915,11 @@ DecodeInsert(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf)
if (FilterByOrigin(ctx, XLogRecGetOrigin(r)))
return;

+ if (ctx->reorder->can_filter_change &&
+ ReorderBufferFilterByRelFileLocator(ctx->reorder,
XLogRecGetXid(r),
+
buf->origptr, &target_locator, true))
+ return;
+

2) Let's add some debug logs so that it will be easy to verify the
changes that are getting filtered, or else we will have to debug and
verify them:
@@ -915,6 +915,11 @@ DecodeInsert(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf)
if (FilterByOrigin(ctx, XLogRecGetOrigin(r)))
return;

+ if (ctx->reorder->can_filter_change &&
+ ReorderBufferFilterByRelFileLocator(ctx->reorder,
XLogRecGetXid(r),
+
buf->origptr, &target_locator, true))
+ return;

3) Also there are no tests currently, probably if we add the above
mentioned debug logs we could add few tests and verify them based on
the logs.

4) Can you elaborate a bit in the commit message why we need to
capture if a transaction has snapshot changes:
Subject: [PATCH v12 1/3] Track transactions with internal snapshot changes

Track transactions which have snapshot changes with a new flag
RBTXN_HAS_SNAPSHOT_CHANGES

Regards,
Vignesh