Re: Skip collecting decoded changes of already-aborted transactions

On Thu, Dec 19, 2024 at 9:36 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Dec 20, 2024 at 12:42 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Dec 19, 2024 at 2:56 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > >
> > > @@ -2067,7 +2143,7 @@ ReorderBufferResetTXN(ReorderBuffer *rb,
> > > ReorderBufferTXN *txn,
> > > ReorderBufferChange *specinsert)
> > > {
> > > /* Discard the changes that we just streamed */
> > > - ReorderBufferTruncateTXN(rb, txn, rbtxn_prepared(txn));
> > > + ReorderBufferTruncateTXN(rb, txn, rbtxn_prepared(txn), true);
> > >
> > > @@ -1924,7 +2000,7 @@ ReorderBufferStreamCommit(ReorderBuffer *rb,
> > > ReorderBufferTXN *txn)
> > > * full cleanup will happen as part of the COMMIT PREPAREDs, so now
> > > * just truncate txn by removing changes and tuplecids.
> > > */
> > > - ReorderBufferTruncateTXN(rb, txn, true);
> > > + ReorderBufferTruncateTXN(rb, txn, true, true);
> > >
> > > In both the above places, the patch unconditionally passes the
> > > 'txn_streaming' even for prepared transactions when it wouldn't be a
> > > streaming xact. Inside the function, the patch handled that by first
> > > checking whether the transaction is prepared (txn_prepared). So, the
> > > logic will work but the function signature and the way its callers are
> > > using make it difficult to use and extend in the future.
> > >
> >
> > Valid concern.
> >
> > > I think for the first case, we should get the streaming parameter in
> > > ReorderBufferResetTXN(),
> >
> > I think we cannot pass 'rbtxn_is_streamed(txn)' to
> > ReorderBufferTruncateTXN() in the first case. ReorderBufferResetTXN()
> > is called to handle the concurrent abort of the streaming transaction
> > but the transaction might not have been marked as streamed at that
> > time. Since ReorderBufferTruncateTXN() is responsible for both
> > discarding changes and marking the transaction as streamed, we need to
> > unconditionally pass txn_streaming = true in this case.
> >
>
> Can't we use 'stream_started' variable available at the call site of
> ReorderBufferResetTXN() for our purpose?

Right, we can use it.

>
> >
> > On second thoughts, I think the confusion related to txn_streaming
> > came from the fact that ReorderBufferTruncateTXN() does both
> > discarding changes and marking the transaction as streamed. If we make
> > the function do just discarding changes, we don't need to introduce
> > the txn_streaming function argument. Instead, we need to have a
> > separate function to mark the transaction as streamed and call it
> > before ReorderBufferTruncateTXN() where appropriate. And
> > ReorderBufferCheckAndTruncateAbortedTXN() just calls
> > ReorderBufferTruncateTXN().
> >
>
> That sounds good to me. IIRC, initially, ReorderBufferTruncateTXN()
> was used to truncate changes only for streaming transactions. Later,
> it evolved for prepared facts and now for facts where we explicitly
> detect whether they are aborted. So, I think it makes sense to improve
> it by following your suggestion.

I've changed the patch accordingly.

>
> >
> > >
> > > *
> > > + * 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);
> > > +
> > > + /* All changes should be discarded */
> > > + Assert(txn->size == 0);
> > >
> > > Can we expect the size to be zero without resetting the toast data? In
> > > ReorderBufferToastReset(), we call ReorderBufferReturnChange() which
> > > reduces the change size. So, won't that size still be accounted for in
> > > txn?
> >
> > IIUC the toast reconstruction data is created only while replaying the
> > transaction but the ReorderBufferCheckAndTruncateAbortedTXN() is not
> > called during that. So I think any toast data should not be
> > accumulated at that time.
> >
>
> How about the case where in the first pass, we streamed the
> transaction partially, where it has reconstructed toast data, and
> then, in the second pass, when memory becomes full, the reorder buffer
> contains some partial data, due to which it tries to spill the data
> and finds that the transaction is aborted? I could be wrong here
> because I haven't tried to test this code path, but I see that it is
> theoretically possible.

Yeah, it seems possible. I've changed the patch to reset toast data as well.

Regards,

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