Re: Skip collecting decoded changes of already-aborted transactions

On Tue, 26 Nov 2024 at 02:58, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> 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?

I have checked further regarding the toast and verified the population
of the toast hash. I agree with you on this. Overall, the patch
appears to be in good shape.

Regards,
Vignesh