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

On Mon, Oct 26, 2020 at 6:49 PM Peter Smith <smithpb2250@gmail.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