Re: [HACKERS] logical decoding of two-phase transactions

On Wed, Nov 11, 2020 at 12:35 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
I have rearranged the code in DecodeCommit/Abort/Prepare so
> that it does only the required things (like in DecodeCommit was still
> processing subtxns even when it has to just perform FinishPrepared,
> also the stats were not updated properly which I have fixed.) and
> added/edited the comments. Apart from 0001 and 0002, I have not
> changed anything in the remaining patches.

One small comment on the patch:

- DecodeCommit(ctx, buf, &parsed, xid);
+ /*
+ * If we have already decoded this transaction data then
+ * DecodeCommit doesn't need to decode it again. This is
+ * possible iff output plugin supports two-phase commits and
+ * doesn't skip the transaction at prepare time.
+ */
+ if (info == XLOG_XACT_COMMIT_PREPARED && ctx->twophase)
+ {
+ already_decoded = !(ctx->callbacks.filter_prepare_cb &&
+ ReorderBufferPrepareNeedSkip(ctx->reorder, xid, parsed.twophase_gid));
+ }
+

Just a small nitpick but the way already_decoded is assigned here is a
bit misleading. It appears that the callbacks determine if the
transaction is already decoded when in
reality the callbacks only decide if the transaction should skip two
phase commits. I think it's better to either move it to the if
condition or if that is too long then have one more variable
skip_twophase.

if (info == XLOG_XACT_COMMIT_PREPARED && ctx->twophase &&
!(ctx->callbacks.filter_prepare_cb &&
ReorderBufferPrepareNeedSkip(ctx->reorder, xid, parsed.twophase_gid)))
already_decoded = true;

OR
bool skip_twophase = false;
skip_twophase = !(ctx->callbacks.filter_prepare_cb &&
ReorderBufferPrepareNeedSkip(ctx->reorder, xid, parsed.twophase_gid));
if (info == XLOG_XACT_COMMIT_PREPARED && ctx->twophase && skip_twophase)
already_decoded = true;

regards,
Ajin Cherian
Fujitsu Australia