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-19T13:24:23Z
Lists: pgsql-hackers 
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?

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.

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