Re: Skip collecting decoded changes of already-aborted transactions

On Tue, 26 Nov 2024 at 02:58, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Nov 18, 2024 at 11:12 PM vignesh C <vignesh21@gmail.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