On Wed, Nov 11, 2020 at 12:35 AM Amit Kapila <amit.kapil...@gmail.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