Re: Skip collecting decoded changes of already-aborted transactions

On Tue, Dec 10, 2024 at 10:39 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 5.
> +/*
> + * 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.
> + */
> +static bool
> +ReorderBufferTruncateTXNIfAborted(ReorderBuffer *rb, ReorderBufferTXN *txn)
> +{
>
> I think this function is being invoked to mark a sub-transaction as
> aborted. It is better to explain in comments how it interacts with
> sub-transactions, why it is okay to mark them as aborted, and how the
> other parts of the system interact with it.
>

The current name suggests that the main purpose is to truncate the txn
which is okay but wouldn't it be better to name on the lines of
ReorderBufferCheckAndTruncateAbortedTXN()?

In the following comment, can we move 'Return ...' to the next line to
make the return values from the function clear?
+ * next call. Return true if the transaction is aborted, otherwise return
+ * false.

--
With Regards,
Amit Kapila.