Re: Skip collecting decoded changes of already-aborted transactions

On Mon, Nov 18, 2024 at 11:12 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
>
> Few comments:

Thank you for reviewing the patch!

> 1) Should we have the Assert inside ReorderBufferTruncateTXNIfAborted
> instead of having it at multiple callers, ReorderBufferResetTXN also
> has the Assert inside the function after truncate of the transaction:
> @@ -3672,6 +3758,14 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
> Assert(txn->total_size > 0);
> Assert(rb->size >= txn->total_size);
>
> + /* skip the transaction if aborted */
> + if (ReorderBufferTruncateTXNIfAborted(rb, txn))
> + {
> + /* All changes should be discarded */
> + Assert(txn->size == 0 && txn->total_size == 0);
> + continue;
> + }
> +
> ReorderBufferStreamTXN(rb, txn);
> }
> else
> @@ -3687,6 +3781,14 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
> Assert(txn->size > 0);
> Assert(rb->size >= txn->size);
>
> + /* skip the transaction if aborted */
> + if (ReorderBufferTruncateTXNIfAborted(rb, txn))
> + {
> + /* All changes should be discarded */
> + Assert(txn->size == 0 && txn->total_size == 0);
> + continue;
> + }

Moved.

>
> 2) txn->txn_flags can be moved to the next line to keep it within 80
> chars in this case:
> * 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

Fixed.

>
> 3) Is there any scenario where the Assert can fail as the toast is not reset:
> + * Since we don't check the transaction status while replaying the
> + * transaction, we don't need to reset toast reconstruction data here.
> + */
> + ReorderBufferTruncateTXN(rb, txn, rbtxn_prepared(txn), false);
>
> + if (ReorderBufferTruncateTXNIfAborted(rb, txn))
> + {
> + /* All changes should be discarded */
> + Assert(txn->size == 0 && txn->total_size == 0);
> + continue;
> + }

IIUC we reconstruct TOAST data when replaying the transaction. On the
other hand, this function is called while adding a decoded change but
not when replaying the transaction. So we should not have any toast
reconstruction data at this point unless I'm missing something. Do you
have any scenario where we call ReorderBufferTruncateTXNIfAborted()
while a transaction has TOAST reconstruction data?

>
> 4) This can be changed to a single line comment:
> + /*
> + * Quick return if the transaction status is already known.
> + */
> + if (rbtxn_is_committed(txn))
> + return false;

Fixed.

I'll post the updated version patch soon.

Regards,

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