Re: Skip collecting decoded changes of already-aborted transactions

On Mon, Jan 6, 2025 at 5:52 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Sawada-San.
>
> Here are some review comments for the patch v12-0001.

Thank you for reviewing the patch!

>
> ======
> .../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".

Fixed.

>
> ~~~
>
> 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;
> + }

I used a block comment.

>
> ~~~
>
> 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))".

I'm not sure it increases the readability. I think it pretty makes
sense to me that we return false in the 'if
(TransactionIdDidCommit(txn->xid))' block. If we add the 'else' block,
the reader might be confused as we have the 'else' block in spite of
having the return in the 'if' block. We can add a local variable for
the result and return it at the end of the function but I'm not sure
it's a good idea to increase the readability.

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

Hmm, it seems we don't use TOAST at all at least in reorderbuffer.c. I
would prefer to make it consistent with others.

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

I don't understand this comment. What words are you suggesting adding
to these sentences?

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

Fixed.

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

Fixed.

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

Fair point. I've clarified the comments for macros. As for renaming
the existing constants and associated macros, I sent my thoughts in an
email[1] and implemented it in a separate patch (the 0002 patch).

Regards,

[1] https://www.postgresql.org/message-id/CAD21AoBgxqFVKq1yf%2BNR2dHBt47xtkFQ%3DJtxwcAv1PSjTahoPw%40mail.gmail.com

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com