On Wed, Jan 29, 2025 at 9:31 AM Peter Smith <smithpb2...@gmail.com> wrote:
Hi Ajin,
Some review comments for patch v12-0001.
======
Commit message
1.
Track transactions which have snapshot changes with a new flag
RBTXN_HAS_SNAPSHOT_CHANGES
~
The commit message only says *what* it does, but not *why* this patch
even exists. TBH, I don't understand why this patch needs to be
separated from your patch 0002, because 0001 makes no independent use
of the flag, nor is it separately tested.
Anyway, if it is going to remain separated then IMO at least the the
message should explain the intended purpose e.g. why the subsequent
patches require this flagged info and how they will use it.
Fixed.
======
src/include/replication/reorderbuffer.h
2.
+/* Does this transaction have snapshot changes? */
+#define rbtxn_has_snapshot_changes(txn) \
+( \
+ ((txn)->txn_flags & RBTXN_HAS_SNAPSHOT_CHANGES) != 0 \
+)
+
Is the below wording maybe a more plain way to say that:
/* Does this transaction make changes to the current snapshot? */
Fixed as suggested.
On Sat, Feb 1, 2025 at 12:43 AM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
Dear Ajin, Hou,
> - Snapshot construction
I understand the approach that we do not try to filter for all the workloads; do
just best-effort.
I played with your PoC and here are my comments.
1.
Can you add tests for better understanding? I've tried for
tes_decoding like attached,
but it couldn't pass the regression test. Cases "stream.sql" and
"twophase_stream.sql"
were failed.
I've added tests. I've analysed your patch and the regression test
failure. The tests fail to detect concurrent aborts.I think the reason
for that is because of the filter check logic access of the catalog,
the changes are cached.
As a result, when the actual decoding happens, the catalog is not
accessed as the relation detailsare in cache. Without catalog access,
concurrent aborts cannot be detected as concurrent aborts are
detectedonly when the catalog is accessed. There is a new thread by
Sawada-san on a more efficient detection of concurrent aborts,I don't
know if that will solve this issue, otherwise I don't know how to fix
this in a meaningful way. Caching improvesperformance, and at the same
time it prevents detection of concurrent aborts.
2.
I think we can extend the skip mechanism to
UPDATE/DELETE/MultiInsert/SpecConfirm.
Regarding the TRUNCATE, I'm not sure we can handle hte TRUNCATE case
because the we
can't track RelFileLocator anymore.
Updated.
On Tue, Feb 4, 2025 at 2:19 PM vignesh C <vignes...@gmail.com> wrote:
On Tue, 28 Jan 2025 at 08:40, Ajin Cherian <itsa...@gmail.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;
+
Updated.
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.
Updated.
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
Updated.
regards,
Ajin Cherian
Fujitsu Australia
v13-0003-Introduce-a-output-plugin-callback-to-filter-cha.patch
Description: Binary data
v13-0001-Track-transactions-with-internal-snapshot-change.patch
Description: Binary data
v13-0002-Filter-transactions-that-need-not-be-published.patch
Description: Binary data
