Horton

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

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: Ajin Cherian <itsajin@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Masahiko Sawada <sawada.mshk@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2020-09-21T04:22:16Z
Lists: pgsql-hackers 
On Fri, Sep 18, 2020 at 6:02 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Sep 15, 2020 at 10:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > I don't think it is complete yet.
> > *
> > * This error can only occur when we are sending the data in
> >   * streaming mode and the streaming is not finished yet.
> >   */
> > - Assert(streaming);
> > - Assert(stream_started);
> > + Assert(streaming || rbtxn_prepared(txn));
> > + Assert(stream_started  || rbtxn_prepared(txn));
> >
> > Here, you have updated the code but comments are still not updated.
> >
>
> Updated the comments.
>
>
> > I don't think we need to perform abort here. Later we will anyway
> > encounter the WAL for Rollback Prepared for which we will call
> > abort_prepared_cb. As we have set the 'concurrent_abort' flag, it will
> > allow us to skip all the intermediate records. Here, we need only
> > enough state in ReorderBufferTxn that it can be later used for
> > ReorderBufferFinishPrepared(). Basically, you need functionality
> > similar to ReorderBufferTruncateTXN where except for invalidations you
> > can free memory for everything else. You can either write a new
> > function ReorderBufferTruncatePreparedTxn or pass another bool
> > parameter in ReorderBufferTruncateTXN to indicate it is prepared_xact
> > and then clean up additional things that are not required for prepared
> > xact.
>
> Added a new parameter to ReorderBufferTruncatePreparedTxn  for
> prepared transactions and did cleanup of tupulecids as well, I have
> left snapshots and transactions.
> As a result of this, I also had to create a new function
> ReorderBufferCleanupPreparedTXN which will clean up the rest as part
> of FinishPrepared handling as we can't call
> ReorderBufferCleanupTXN again after this.
>

Why can't we call ReorderBufferCleanupTXN() from
ReorderBufferFinishPrepared after your changes?

+ * If streaming, keep the remaining info - transactions, tuplecids,
invalidations and
+ * snapshots.If after a PREPARE, keep only the invalidations and snapshots.
  */
 static void
-ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
+ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
bool txn_prepared)

Why do we need even snapshot for Prepared transactions? Also, note
that in the comment there is no space before you start a new line.

>
> > >>
> > >>
> > >> I don't know why the patch has used this way to implement an option to
> > >> enable two-phase. Can't we use how we implement 'stream-changes'
> > >> option in commit 7259736a6e? Just refer how we set ctx->streaming and
> > >> you can use a similar way to set this parameter.
> > >
> > >
> > > Done, I've moved the checks for callbacks to inside the corresponding wrappers.
> > >
> >
> > This is not what I suggested. Please study the commit 7259736a6e and
> > see how streaming option is implemented. I want later subscribers can
> > specify whether they want transactions to be decoded at prepare time
> > similar to what we have done for streaming. Also, search for
> > ctx->streaming in the code and see how it is set to get the idea.
> >
>
> Changed it similar to ctx->streaming logic.
>

Hmm, I still don't see changes relevant changes in pg_decode_startup().

-- 
With Regards,
Amit Kapila.

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