Re: [HACKERS] logical decoding of two-phase transactions - Mailing list pgsql-hackers

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.

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.

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.

-- 
With Regards,
Amit Kapila.