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

On Mon, Nov 2, 2020 at 9:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Oct 28, 2020 at 10:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Ajin.
> >
> > I have re-checked the v13 patches for how my remaining review comments
> > have been addressed.
> >
> > On Tue, Oct 27, 2020 at 8:55 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > > ====================
> > > > v12-0002. File: src/backend/replication/logical/reorderbuffer.c
> > > > ====================
> > > >
> > > > COMMENT
> > > > Line 2401
> > > > /*
> > > >  * We are here due to one of the 3 scenarios:
> > > >  * 1. As part of streaming in-progress transactions
> > > >  * 2. Prepare of a two-phase commit
> > > >  * 3. Commit of a transaction.
> > > >  *
> > > >  * If we are streaming the in-progress transaction then discard the
> > > >  * changes that we just streamed, and mark the transactions as
> > > >  * streamed (if they contained changes), set prepared flag as false.
> > > >  * If part of a prepare of a two-phase commit set the prepared flag
> > > >  * as true so that we can discard changes and cleanup tuplecids.
> > > >  * Otherwise, remove all the
> > > >  * changes and deallocate the ReorderBufferTXN.
> > > >  */
> > > > ~
> > > > The above comment is beyond my understanding. Anything you could do to
> > > > simplify it would be good.
> > > >
> > > > For example, when viewing this function in isolation I have never
> > > > understood why the streaming flag and rbtxn_prepared(txn) flag are not
> > > > possible to be set at the same time?
> > > >
> > > > Perhaps the code is relying on just internal knowledge of how this
> > > > helper function gets called? And if it is just that, then IMO there
> > > > really should be some Asserts in the code to give more assurance about
> > > > that. (Or maybe use completely different flags to represent those 3
> > > > scenarios instead of bending the meanings of the existing flags)
> > > >
> > >
> > > Left this for now, probably re-look at this at a later review.
> > > But just to explain; this function is what does the main decoding of
> > > changes of a transaction.
> > >  At what point this decoding happens is what this feature and the
> > > streaming in-progress feature is about. As of PG13, this decoding only
> > > happens at commit time. With the streaming of in-progress txn feature,
> > > this began to happen (if streaming enabled) at the time when the
> > > memory limit for decoding transactions was crossed. This 2PC feature
> > > is supporting decoding at the time of a PREPARE transaction.
> > > Now, if streaming is enabled and streaming has started as a result of
> > > crossing the memory threshold, then there is no need to
> > > again begin streaming at a PREPARE transaction as the transaction that
> > > is being prepared has already been streamed.
> > >
>
> I don't think this is true, think of a case where we need to send the
> last set of changes along with PREPARE. In that case we need to stream
> those changes at the time of PREPARE. If I am correct then as pointed
> by Peter you need to change some comments and some of the assumptions
> related to this you have in the patch.

I have changed the asserts and the comments to reflect this.

>
> Few more comments on the latest patch
> (v15-0002-Support-2PC-txn-backend-and-tests)
> =========================================================================
> 1.
> @@ -274,6 +296,23 @@ DecodeXactOp(LogicalDecodingContext *ctx,
> XLogRecordBuffer *buf)
>   DecodeAbort(ctx, buf, &parsed, xid);
>   break;
>   }
> + case XLOG_XACT_ABORT_PREPARED:
> + {
>
> ..
> +
> + if (!TransactionIdIsValid(parsed.twophase_xid))
> + xid = XLogRecGetXid(r);
> + else
> + xid = parsed.twophase_xid;
>
> I think we don't need this 'if' check here because you must have a
> valid value of parsed.twophase_xid;. But, I think this will be moot if
> you address the review comment in my previous email such that the
> handling of XLOG_XACT_ABORT_PREPARED and XLOG_XACT_ABORT will be
> combined as it is there without the patch.
>
> 2.
> +DecodePrepare(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
> +   xl_xact_parsed_prepare * parsed)
> +{
> ..
> + if (SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) ||
> + (parsed->dbId != InvalidOid && parsed->dbId != ctx->slot->data.database) ||
> + ctx->fast_forward || FilterByOrigin(ctx, origin_id))
> + return;
> +
>
> I think this check is the same as the check in DecodeCommit, so you
> can write some comments to indicate the same and also why we don't
> need to call ReorderBufferForget here. One more thing is to note is
> even if we don't need to call ReorderBufferForget here but still we
> need to execute invalidations (which are present in top-level txn) for
> the reasons mentioned in ReorderBufferForget. Also, if we do this,
> don't forget to update the comment atop
> ReorderBufferImmediateInvalidation.

I have updated the comments. I wasn't sure of when to execute
invalidations. Should I only
execute invalidations if this was for another database than what was
being decoded or should
I execute invalidation every time we skip? I will also have to create
a new function in reorderbuffer,c similar to ReorderBufferForget
as the txn is not available in decode.c.

>
> 3.
> + /* This is a PREPARED transaction, part of a two-phase commit.
> + * The full cleanup will happen as part of the COMMIT PREPAREDs, so now
> + * just truncate txn by removing changes and tuple_cids
> + */
> + ReorderBufferTruncateTXN(rb, txn, true);
>
> The first line in the multi-line comment should be empty.

Updated.

regards,
Ajin Cherian
Fujitsu Australia