Re: Skip collecting decoded changes of already-aborted transactions

On Tue, Dec 10, 2024 at 11:09 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Dec 10, 2024 at 10:59 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Tue, Nov 26, 2024 at 3:02 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > >
> > > I've attached a new version patch that incorporates all comments I got so far.
> > >
> > > I think the patch is in good shape but I'm considering whether we
> > > might want to call ReorderBufferToastReset() after truncating all
> > > changes, in ReorderBufferTruncateTXNIfAborted() just in case. Will
> > > investigate further.
> > >
> >
> > There’s something that seems a bit odd to me. Consider the case where
> > the largest transaction(s) are aborted. If
> > ReorderBufferCanStartStreaming() returns true, the changes from this
> > transaction will only be discarded if it's a streamable transaction.
> > However, if ReorderBufferCanStartStreaming() is false, the changes
> > will be discarded regardless.
> >
> > What seems strange to me in this patch is truncating the changes of a
> > large aborted transaction depending on whether we need to stream or
> > spill but actually that should be completely independent IMHO. My
> > concern is that if the largest transaction is aborted but isn’t yet
> > streamable, we might end up picking the next transaction, which could
> > be much smaller. This smaller transaction might not help us stay
> > within the memory limit, and we could repeat this process for a few
> > more transactions. In contrast, it might be more efficient to simply
> > discard the large aborted transaction, even if it’s not streamable, to
> > avoid this issue.
> >
>
> If the largest transaction is non-streamable, won't the transaction
> returned by ReorderBufferLargestTXN() in the other case already
> suffice the need?

I see your point, but I don’t think it’s quite the same. When
ReorderBufferCanStartStreaming() is true, the function
ReorderBufferLargestStreamableTopTXN() looks for the largest
transaction among those that have a base_snapshot. So, if the largest
transaction is aborted but hasn’t yet received a base_snapshot, it
will instead select the largest transaction that does have a
base_snapshot, which could be significantly smaller than the largest
aborted transaction.

I’m not saying this is a very common scenario, but I do feel that the
logic behind truncating the largest transaction doesn’t seem entirely
consistent. However, maybe this isn't a major issue. We could justify
the current behavior by saying that before picking any transaction for
streaming or spilling, we first check whether it has been aborted.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com