Horton

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

Peter Smith <smithpb2250@gmail.com>

View thread View on postgr.es
From: Peter Smith <smithpb2250@gmail.com>
To: Ajin Cherian <itsajin@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2020-10-28T05:20:32Z
Lists: pgsql-hackers 
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

Commits

  1. Add prepare API support for streaming transactions in logical replication.

  2. Doc: minor improvements for logical replication protocol documentation.

  3. Fix test failure in 021_twophase.pl.

  4. Refactor to make common functions in proto.c and worker.c.

  5. Unify PostgresNode's new() and get_new_node() methods

  6. Fix potential buffer overruns in proto.c.

  7. Add support for prepared transactions to built-in logical replication.

  8. Refactor function parse_subscription_options.

  9. Allow enabling two-phase option via replication protocol.

  10. Don't use Asserts to check for violations of replication protocol.

  11. Improve psql tab completion for options of subcriptions and publications

  12. Rearrange logrep worker's snapshot handling some more.

  13. doc: Update information of new messages for logical replication.

  14. ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION

  15. Allow pgoutput to send logical decoding messages.

  16. Refactor function parse_output_parameters.

  17. Avoid repeated decoding of prepared transactions after a restart.

  18. Fix an oversight in ReorderBufferFinishPrepared.

  19. Allow multiple xacts during table sync in logical replication.

  20. Fix replication of in-progress transactions in tablesync worker.

  21. Fix 'skip-empty-xacts' option in test_decoding for streaming mode.

  22. Use Enum for top level logical replication message types.

  23. Add support for streaming to built-in logical replication.

  24. Fix the logical streaming test.

  25. Implement streaming mode in ReorderBuffer.

  26. Extend the logical decoding output plugin API with stream methods.

  27. Store 2PC GID in commit/abort WAL recs for logical decoding