From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Ajin Cherian <itsajin(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] logical decoding of two-phase transactions |
Date: | 2020-10-29 00:48:46 |
Message-ID: | CAHut+Pv91JFrRoAGE-69U21ZMAKoze3LnG5sBxVsqAyOsf2kHQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Smith | 2020-10-29 01:10:40 | Re: [HACKERS] logical decoding of two-phase transactions |
Previous Message | Michael Paquier | 2020-10-29 00:39:42 | Re: Online checksums verification in the backend |