On Tue, Dec 10, 2024 at 11:09 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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?
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
