Re: Skip collecting decoded changes of already-aborted transactions

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?

>
> 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.

>
> >
> > *
> > + * 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.

--
With Regards,
Amit Kapila.