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

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>

To: Ajin Cherian <itsajin@gmail.com>

Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>

Date: 2020-10-29T00:48:46Z

Lists: pgsql-hackers

Hi Ajin.

Looking at v13 patches again I found a couple more review comments:

===

(1) COMMENT
File: src/backend/replication/logical/proto.c
Function: logicalrep_write_prepare
+ if (rbtxn_commit_prepared(txn))
+ flags = LOGICALREP_IS_COMMIT_PREPARED;
+ else if (rbtxn_rollback_prepared(txn))
+ flags = LOGICALREP_IS_ROLLBACK_PREPARED;
+ else
+ flags = LOGICALREP_IS_PREPARE;
+
+ /* Make sure exactly one of the expected flags is set. */
+ if (!PrepareFlagsAreValid(flags))
+ elog(ERROR, "unrecognized flags %u in prepare message", flags);

Since those flags are directly assigned, I think the subsequent if
(!PrepareFlagsAreValid(flags)) check is redundant.

===

(2) COMMENT
File: src/backend/replication/logical/proto.c
Function: logicalrep_write_stream_prepare
+/*
+ * Write STREAM PREPARE to the output stream.
+ * (For stream PREPARE, stream COMMIT PREPARED, stream ROLLBACK PREPARED)
+ */

I think the function comment is outdated because IIUC the stream
COMMIT PREPARED and stream ROLLBACK PREPARED are not being handled by
the function logicalrep_write_prepare. SInce this approach seems
counter-intuitive there needs to be an improved function comment to
explain what is going on.

===

(3) COMMENT
File: src/backend/replication/logical/proto.c
Function: logicalrep_read_stream_prepare
+/*
+ * Read STREAM PREPARE from the output stream.
+ * (For stream PREPARE, stream COMMIT PREPARED, stream ROLLBACK PREPARED)
+ */

This is the same as the previous review comment. The function comment
needs to explain the new handling for stream COMMIT PREPARED and
stream ROLLBACK PREPARED.

===

(4) COMMENT
File: src/backend/replication/logical/proto.c
Function: logicalrep_read_stream_prepare
+TransactionId
+logicalrep_read_stream_prepare(StringInfo in, LogicalRepPrepareData
*prepare_data)
+{
+ TransactionId xid;
+ uint8 flags;
+
+ xid = pq_getmsgint(in, 4);
+
+ /* read flags */
+ flags = pq_getmsgbyte(in);
+
+ if (!PrepareFlagsAreValid(flags))
+ elog(ERROR, "unrecognized flags %u in prepare message", flags);

I think the logicalrep_write_stream_prepare now can only assign the
flags = LOGICALREP_IS_PREPARE. So that means the check here for bad
flags should be changed to match.
BEFORE: if (!PrepareFlagsAreValid(flags))
AFTER: if (flags != LOGICALREP_IS_PREPARE)

===

(5) COMMENT
General
Since the COMMENTs (2), (3) and (4) are all caused by the refactoring
that was done for removal of the commit/rollback stream callbacks. I
do wonder if it might have worked out better just to leave the
logicalrep_read/write_stream_prepared as it was instead of mixing up
stream/no-stream handling. A check for stream/no-stream could possibly
have been made higher up.

For example:
static void
pgoutput_commit_prepared_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn,
                             XLogRecPtr prepare_lsn)
{
    OutputPluginUpdateProgress(ctx);

    OutputPluginPrepareWrite(ctx, true);
if (ctx->streaming)
logicalrep_write_stream_prepare(ctx->out, txn, prepare_lsn);
else
logicalrep_write_prepare(ctx->out, txn, prepare_lsn);
    OutputPluginWrite(ctx, true);
}

===

Kind Regards,
Peter Smith.
Fujitsu Australia

Commits

Add prepare API support for streaming transactions in logical replication.
- 63cf61cdeb7b 15.0 landed
Doc: minor improvements for logical replication protocol documentation.
- a5cb4f9829fb 15.0 cited
Fix test failure in 021_twophase.pl.
- eaf5321c3524 15.0 landed
Refactor to make common functions in proto.c and worker.c.
- 91f9861242cd 15.0 landed
Unify PostgresNode's new() and get_new_node() methods
- 201a76183e20 15.0 cited
Fix potential buffer overruns in proto.c.
- 01c3adcdd85f 15.0 landed
Add support for prepared transactions to built-in logical replication.
- a8fd13cab0ba 15.0 cited
Refactor function parse_subscription_options.
- 8aafb0261675 15.0 cited
Allow enabling two-phase option via replication protocol.
- cda03cfed6b8 15.0 cited
Don't use Asserts to check for violations of replication protocol.
- fe6a20ce54cb 14.0 cited
Improve psql tab completion for options of subcriptions and publications
- d08237b5b494 14.0 cited
Rearrange logrep worker's snapshot handling some more.
- 3a09d75b4f6c 14.0 cited
doc: Update information of new messages for logical replication.
- 15c1a9d9cb76 14.0 cited
ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
- 82ed7748b710 14.0 cited
Allow pgoutput to send logical decoding messages.
- ac4645c0157f 14.0 cited
Refactor function parse_output_parameters.
- 531737ddad21 14.0 cited
Avoid repeated decoding of prepared transactions after a restart.
- 8bdb1332eb51 14.0 cited
Fix an oversight in ReorderBufferFinishPrepared.
- ade89ba5f408 14.0 landed
Allow multiple xacts during table sync in logical replication.
- ce0fdbfe9722 14.0 cited
Fix replication of in-progress transactions in tablesync worker.
- 0926e96c4934 14.0 cited
Fix 'skip-empty-xacts' option in test_decoding for streaming mode.
- 9653f24ad830 14.0 cited
Use Enum for top level logical replication message types.
- 644f0d7cc9c2 14.0 cited
Add support for streaming to built-in logical replication.
- 464824323e57 14.0 cited
Fix the logical streaming test.
- 82a0ba7707e0 14.0 cited
Implement streaming mode in ReorderBuffer.
- 7259736a6e5b 14.0 cited
Extend the logical decoding output plugin API with stream methods.
- 45fdc9738b36 14.0 cited
Store 2PC GID in commit/abort WAL recs for logical decoding
- 1eb6d6527aae 11.0 cited