On Tue, Dec 10, 2024 at 10:59 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Tue, Nov 26, 2024 at 3:02 AM Masahiko Sawada <sawada.m...@gmail.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? -- With Regards, Amit Kapila.
