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

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. Which is why this
> function will not be called when a streaming transaction is prepared
> as part of a two-phase commit.

AFAIK the last remaining issue now is only about the complexity of the
aforementioned code/comment. If you want to defer changing that until
we can come up with something better, then that is OK by me.

Apart from that I have no other pending review comments at this time.

Kind Regards,
Peter Smith.
Fujitsu Australia