Horton

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

Greg Nancarrow <gregn4422@gmail.com>

View thread View on postgr.es
From: Greg Nancarrow <gregn4422@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Ajin Cherian <itsajin@gmail.com>, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com>, vignesh C <vignesh21@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2021-06-09T04:28:28Z
Lists: pgsql-hackers 
On Tue, Jun 8, 2021 at 4:12 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Please find attached the latest patch set v83*
>

Some feedback for the v83 patch set:

v83-0001:

(1) doc/src/sgml/protocol.sgml

(i) Remove extra space:

BEFORE:
+         The transaction will be  decoded and transmitted at
AFTER:
+         The transaction will be decoded and transmitted at

(ii)
BEFORE:
+   contains Stream Commit or Stream Abort message.
AFTER:
+   contains a Stream Commit or Stream Abort message.

(iii)
BEFORE:
+                The LSN of the commit prepared.
AFTER:
+                The LSN of the commit prepared transaction.

(iv) Should documentation say "prepared transaction" as opposed to
"prepare transaction" ???

BEFORE:
+                The end LSN of the prepare transaction.
AFTER:
+                The end LSN of the prepared transaction.


(2) doc/src/sgml/ref/create_subscription.sgml

(i)
BEFORE:
+          The <literal>streaming</literal> option cannot be used along with
+          <literal>two_phase</literal> option.
AFTER:
+          The <literal>streaming</literal> option cannot be used with the
+          <literal>two_phase</literal> option.


(3) doc/src/sgml/ref/create_subscription.sgml

(i)
BEFORE:
+          prepared on publisher is decoded as normal transaction at commit.
AFTER:
+          prepared on the publisher is decoded as a normal
transaction at commit.

(ii)
BEFORE:
+          The <literal>two_phase</literal> option cannot be used along with
+          <literal>streaming</literal> option.
AFTER:
+          The <literal>two_phase</literal> option cannot be used with the
+          <literal>streaming</literal> option.


(4) src/backend/access/transam/twophase.c

(i)
BEFORE:
+ * Check if the prepared transaction with the given GID, lsn and timestamp
+ * is around.
AFTER:
+ * Check if the prepared transaction with the given GID, lsn and timestamp
+ * exists.


(5) src/backend/access/transam/twophase.c

Question:

Is:

+ * do this optimization if we encounter many collisions in GID

meant to be:

+ * do this optimization if we encounter any collisions in GID

???

(6) src/backend/replication/logical/decode.c

Grammar:

BEFORE:
+ * distributed 2PC. This can be avoided by disallowing to
+ * prepare transactions that have locked [user] catalog tables
+ * exclusively but as of now we ask users not to do such
+ * operation.
AFTER:
+ * distributed 2PC. This can be avoided by disallowing
+ * prepared transactions that have locked [user] catalog tables
+ * exclusively but as of now we ask users not to do such an
+ * operation.


(7) src/backend/replication/logical/logical.c

From the comment above it, it's not clear if the "&=" in the following
line is intentional:

+ ctx->twophase &= (ctx->twophase_opt_given || slot->data.two_phase);

Also, the boolean conditions tested are in the reverse order of what
is mentioned in that comment.
Based on the comment, I would expect the following code:

+ ctx->twophase = (slot->data.two_phase || ctx->twophase_opt_given);

Please check it, and maybe update the comment if "&=" is really intended.

There are TWO places where this same code is used.

(8) src/backend/replication/logical/tablesync.c

In the following code, "has_subrels" should be a bool, not an int.

+static bool
+FetchTableStates(bool *started_tx)
+{
+ static int has_subrels = false;


(9) src/backend/replication/logical/worker.c

Mixed current/past tense:

BEFORE:
+ * was still busy (see the condition of should_apply_changes_for_rel). The
AFTER:
+ * is still busy (see the condition of should_apply_changes_for_rel). The

(10)

2 places:

BEFORE:
+ /* there is no transaction when COMMIT PREPARED is called */
AFTER:
+ /* There is no transaction when COMMIT PREPARED is called */


v83-0002:

1) doc/src/sgml/protocol.sgml

BEFORE:
+   contains Stream Prepare or Stream Commit or Stream Abort message.
AFTER:
+   contains a Stream Prepare or Stream Commit or Stream Abort message.


v83-0003:

1) src/backend/replication/pgoutput/pgoutput.c

i) In pgoutput_commit_txn(), the following code that pfree()s a
pointer in a struct, without then NULLing it out, seems dangerous to
me (because what is to stop other code, either now or in the future,
from subsequently referencing that freed data or perhaps trying to
pfree() again?):

+ PGOutputTxnData *data = (PGOutputTxnData *) txn->output_plugin_private;
+ bool            skip;
+
+ Assert(data);
+ skip = !data->sent_begin_txn;
+ pfree(data);


I suggest adding the following line of code after the pfree():
+ txn->output_plugin_private = NULL;


ii) In pgoutput_commit_prepared_txn(), there's the same type of code:

+ if (data)
+ {
+ bool skip = !data->sent_begin_txn;
+ pfree(data);
+ if (skip)
+ return;
+ }

I suggest adding the following line after the pfree() above:

+ txn->output_plugin_private = NULL;


iii) Again, same thing in pgoutput_rollback_prepared_txn():

I suggest adding the following line after the pfree() above:

+ txn->output_plugin_private = NULL;


Regards,
Greg Nancarrow
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