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

FYI - Please find attached code coverage reports which I generated
(based on the v12 patches) after running the following tests:

1. cd contrib/test_decoding; make check

2. cd src/test/subscriber; make check

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Oct 27, 2020 at 8:55 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Mon, Oct 26, 2020 at 6:49 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Ajin.
> >
> > I checked the to see how my previous review comments (of v10) were
> > addressed by the latest patches (currently v12)
> >
> > There are a couple of remaining items.
> >
> > ---
> >
> > ====================
> > v12-0001. File: doc/src/sgml/logicaldecoding.sgml
> > ====================
> >
> > COMMENT
> > Section 49.6.1
> > Says:
> > An output plugin may also define functions to support streaming of
> > large, in-progress transactions. The stream_start_cb, stream_stop_cb,
> > stream_abort_cb, stream_commit_cb, stream_change_cb, and
> > stream_prepare_cb are required, while stream_message_cb and
> > stream_truncate_cb are optional.
> >
> > An output plugin may also define functions to support two-phase
> > commits, which are decoded on PREPARE TRANSACTION. The prepare_cb,
> > commit_prepared_cb and rollback_prepared_cb callbacks are required,
> > while filter_prepare_cb is optional.
> > ~
> > I was not sure how the paragraphs are organised. e.g. 1st seems to be
> > about streams and 2nd seems to be about two-phase commit. But they are
> > not mutually exclusive, so I guess I thought it was odd that
> > stream_prepare_cb was not mentioned in the 2nd paragraph.
> >
> > Or maybe it is OK as-is?
> >
>
> I've added stream_prepare_cb to the 2nd paragraph as well.
>
>
> > ====================
> > v12-0002. File: contrib/test_decoding/expected/two_phase.out
> > ====================
> >
> > COMMENT
> > Line 26
> > PREPARE TRANSACTION 'test_prepared#1';
> > --
> > SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
> > NULL, 'two-phase-commit', '1', 'include-xids', '0',
> > 'skip-empty-xacts', '1');
> > ~
> > Seems like a missing comment to explain the expectation of that select.
> >
> > ---
> >
>
> Updated.
>
> > COMMENT
> > Line 80
> > -- The insert should show the newly altered column.
> > ~
> > Do you also need to mention something about the DDL not being present
> > in the decoding?
> >
>
> Updated.
>
> > ====================
> > v12-0002. File: src/backend/replication/logical/reorderbuffer.c
> > ====================
> >
> > COMMENT
> > Line 1807
> > /* Here we are streaming and part of the PREPARE 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
> > */
> > ~
> > Something seems strange about the first sentence of that comment
> >
> > ---
> >
> > COMMENT
> > Line 1944
> > /* Discard the changes that we just streamed.
> > * This can only be called if streaming and not part of a PREPARE in
> > * a two-phase commit, so set prepared flag as false.
> > */
> > ~
> > I thought since this comment that is asserting various things, that
> > should also actually be written as code Assert.
> >
> > ---
>
> Added an assert.
>
> >
> > 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.
>
> > ====================
> > v12-0003. File: src/backend/access/transam/twophase.c
> > ====================
> >
> > COMMENT
> > Line 557
> > @@ -548,6 +548,33 @@ MarkAsPrepared(GlobalTransaction gxact, bool lock_held)
> > }
> >
> > /*
> > + * LookupGXact
> > + * Check if the prepared transaction with the given GID is around
> > + */
> > +bool
> > +LookupGXact(const char *gid)
> > +{
> > + int i;
> > + bool found = false;
> > ~
> > Alignment of the variable declarations in LookupGXact function
> >
> > ---
>
> Updated.
>
> Amit, I have also updated your comment about removing function
> declaration from commit 1 and I've added it to commit 2. Also removed
> whitespace errors.
>
> regards,
> Ajin Cherian
> Fujitsu Australia