Horton

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

Ajin Cherian <itsajin@gmail.com>

View thread View on postgr.es
From: Ajin Cherian <itsajin@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2020-11-20T03:42:15Z
Lists: pgsql-hackers 
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.

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

regards,
Ajin Cherian
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