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: Peter Smith <smithpb2250@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2020-11-20T03:49:17Z
Lists: pgsql-hackers 
On Fri, Nov 20, 2020 at 9:12 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 12:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Nov 19, 2020 at 2:52 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Thu, Nov 19, 2020 at 5:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > > I think the same check should be there in truncate as well to make the
> > > > APIs consistent and also one can use it for writing another test that
> > > > has a truncate operation.
> > >
> > > Updated the checks in both truncate callbacks (stream and non-stream).
> > > Also added a test case for testing concurrent aborts while decoding
> > > streaming TRUNCATE.
> > >
> >
> > While reviewing/editing the code in 0002-Support-2PC-txn-backend, I
> > came across the following code which seems dubious to me.
> >
> > 1.
> > + /*
> > + * If streaming, reset the TXN so that it is allowed to stream
> > + * remaining data. Streaming can also be on a prepared txn, handle
> > + * it the same way.
> > + */
> > + if (streaming)
> > + {
> > + elog(LOG, "stopping decoding of %u",txn->xid);
> > + ReorderBufferResetTXN(rb, txn, snapshot_now,
> > +   command_id, prev_lsn,
> > +   specinsert);
> > + }
> > + else
> > + {
> > + elog(LOG, "stopping decoding of %s (%u)",
> > + txn->gid != NULL ? txn->gid : "", txn->xid);
> > + ReorderBufferTruncateTXN(rb, txn, true);
> > + }
> >
> > Why do we need to handle the prepared txn case differently here? I
> > think for both cases we can call ReorderBufferResetTXN as it frees the
> > memory we should free in exceptions. Sure, there is some code (like
> > stream_stop and saving the snapshot for next run) in
> > ReorderBufferResetTXN which needs to be only called when we are
> > streaming the txn but otherwise, it seems it can be used here. We can
> > easily identify if the transaction is streamed to differentiate that
> > code path. Can you think of any other reason for not doing so?
>
> Yes, I agree with this that ReorderBufferResetTXN  needs to be called
> to free up memory after an exception.
> Will change ReorderBufferResetTXN so that it now has an extra
> parameter that indicates streaming; so that the stream_stop and saving
> of the snapshot is only done if streaming.
>

I've already made the changes for this in the patch, you can verify
the same when I'll share the new version. We don't need to pass an
extra parameter rbtx_prepared()/rbtxn_is_streamed should serve the
need.

> >
> > 2.
> > +void
> > +ReorderBufferFinishPrepared(ReorderBuffer *rb, TransactionId xid,
> > + XLogRecPtr commit_lsn, XLogRecPtr end_lsn,
> > + TimestampTz commit_time,
> > + RepOriginId origin_id, XLogRecPtr origin_lsn,
> > + char *gid, bool is_commit)
> > +{
> > + ReorderBufferTXN *txn;
> > +
> > + /*
> > + * The transaction may or may not exist (during restarts for example).
> > + * Anyway, two-phase transactions do not contain any reorderbuffers. So
> > + * allow it to be created below.
> > + */
> > + txn = ReorderBufferTXNByXid(rb, xid, true, NULL, commit_lsn,
> > + true);
> >
> > Why should we allow to create a new transaction here or in other words
> > in which cases txn won't be present? I guess this should be the case
> > with the earlier version of the patch where at prepare time we were
> > cleaning the ReorderBufferTxn.
>
> Just confirmed this, yes, you are right. Even after a restart, the
> transaction does get created again prior to this, We need not be
> creating
> it here. I will change this as well.
>

I'll take care of it along with other changes.

Thanks for the confirmation.


-- 
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