Re: Skip collecting decoded changes of already-aborted transactions - Mailing list pgsql-hackers

Hi Sawada-San.

Here are some review comments for the patch v12-0001.

======
.../replication/logical/reorderbuffer.c

ReorderBufferCheckAndTruncateAbortedTXN:

1.
+/*
+ * Check the transaction status by looking CLOG and discard all changes if
+ * the transaction is aborted. The transaction status is cached in
+ * txn->txn_flags so we can skip future changes and avoid CLOG lookups on the
+ * next call.
+ *
+ * Return true if the transaction is aborted, otherwise return false.
+ *
+ * When the 'debug_logical_replication_streaming' is set to "immediate", we
+ * don't check the transaction status, meaning the caller will always process
+ * this transaction.
+ */

Typo "by looking CLOG".

It should be something like "by CLOG lookup".

~~~

2.
+ /* Quick return if the transaction status is already known */
+ if (rbtxn_is_committed(txn))
+ return false;
+ if (rbtxn_is_aborted(txn))
+ {
+ /* Already-aborted transactions should not have any changes */
+ Assert(txn->size == 0);
+
+ return true;
+ }
+

Consider changing that 2nd 'if' to be 'else if', because then that
will make it more obvious that the earlier single line comment "Quick
return if...", in fact applies to both these conditions.

Alternatively, make that a block comment and add some blank lines like:

+ /*
+    * Quick returns if the transaction status is already known.
+    */
+
+ if (rbtxn_is_committed(txn))
+ return false;
+
+ if (rbtxn_is_aborted(txn))
+ {
+ /* Already-aborted transactions should not have any changes */
+ Assert(txn->size == 0);
+
+ return true;
+ }

~~~

3.
+ if (TransactionIdDidCommit(txn->xid))
+ {
+ /*
+ * Remember the transaction is committed so that we can skip CLOG
+ * check next time, avoiding the pressure on CLOG lookup.
+ */
+ Assert(!rbtxn_is_aborted(txn));
+ txn->txn_flags |= RBTXN_IS_COMMITTED;
+ return false;
+ }
+
+ /*
+ * The transaction aborted. We discard the changes we've collected so far
+ * and toast reconstruction data. The full cleanup will happen as part of
+ * decoding ABORT record of this transaction.
+ */
+ ReorderBufferTruncateTXN(rb, txn, rbtxn_prepared(txn));
+ ReorderBufferToastReset(rb, txn);
+
+ /* All changes should be discarded */
+ Assert(txn->size == 0);
+
+ /*
+ * Mark the transaction as aborted so we can ignore future changes of this
+ * transaction.
+ */
+ Assert(!rbtxn_is_committed(txn));
+ txn->txn_flags |= RBTXN_IS_ABORTED;
+
+ return true;
+}

3a.
That whole last part related to "The transaction aborted", might be
clearer if the whole chunk of code was in an 'else' block from the
previous "if (TransactionIdDidCommit(txn->xid))".

~

3b.
"toast" is an acronym so it should be written in uppercase IMO.

~

3c.
The "and toast reconstruction data" seems to be missing a word/s. (??)
- "... and also discard TOAST reconstruction data"
- "... and reset TOAST reconstruction data"

~~~

ReorderBufferMaybeMarkTXNStreamed:

4.
+static void
+ReorderBufferMaybeMarkTXNStreamed(ReorderBuffer *rb, ReorderBufferTXN *txn)
+{
+ /*
+ * The top-level transaction, is marked as streamed always, even if it
+ * does not contain any changes (that is, when all the changes are in
+ * subtransactions).
+ *
+ * For subtransactions, we only mark them as streamed when there are
+ * changes in them.
+ *
+ * We do it this way because of aborts - we don't want to send aborts for
+ * XIDs the downstream is not aware of. And of course, it always knows
+ * about the toplevel xact (we send the XID in all messages), but we never
+ * stream XIDs of empty subxacts.
+ */
+ if (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0))
+ txn->txn_flags |= RBTXN_IS_STREAMED;
+}

/the toplevel xact/the top-level xact/

~~~

5.
  /*
- * We send the prepare for the concurrently aborted xacts so that later
- * when rollback prepared is decoded and sent, the downstream should be
- * able to rollback such a xact. See comments atop DecodePrepare.
- *
- * Note, for the concurrent_abort + streaming case a stream_prepare was
- * already sent within the ReorderBufferReplay call above.
+ * Send a prepare if not yet. It happens if we detected the concurrent
+ * abort while replaying the non-streaming transaction.
  */

The first sentence "if not yet" seems incomplete/missing words.

SUGGESTION
Send a prepare if not already done so. This might occur if we had
detected a concurrent abort while replaying the non-streaming
transaction.

======
src/include/replication/reorderbuffer.h

6.
 #define RBTXN_PREPARE              0x0040
 #define RBTXN_SKIPPED_PREPARE   0x0080
 #define RBTXN_HAS_STREAMABLE_CHANGE 0x0100
+#define RBTXN_SENT_PREPARE 0x0200
+#define RBTXN_IS_COMMITTED 0x0400
+#define RBTXN_IS_ABORTED 0x0800

Something about this new RBTXN_SENT_PREPARE name seems inconsistent to me.

I feel there is now also some introduced ambiguity with these macros:

/* Has this transaction been prepared? */
#define rbtxn_prepared(txn) \
( \
((txn)->txn_flags & RBTXN_PREPARE) != 0 \
)

+/* Has a prepare or stream_prepare already been sent? */
+#define rbtxn_sent_prepare(txn) \
+( \
+ ((txn)->txn_flags & RBTXN_SENT_PREPARE) != 0 \
+)


e.g. It's also not clear from the comments what is the distinction
between the existing macro comment "Has this transaction been
prepared?" and the new macro comment "Has a prepare or stream_prepare
already been sent?".

Indeed, I was wondering if some of the places currently calling
"rbtxn_prepared(txn)" should now strictly be calling
"rbtxn_sent_prepared(txn)" macro instead?

IMO some minor renaming of the existing constants (and also their
associated macros) might help to make all this more coherent. For
example, perhaps like:

#define RBTXN_IS_PREPARE_NEEDED        0x0040
#define RBTXN_IS_PREPARE_SKIPPED   0x0080
#define RBTXN_IS_PREPARE_SENT 0x0200

======
Kind Regards,
Peter Smith.
Fujitsu Australia