On Thu, Dec 19, 2024 at 9:36 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Fri, Dec 20, 2024 at 12:42 AM Masahiko Sawada <sawada.m...@gmail.com>
> wrote:
> >
> > On Thu, Dec 19, 2024 at 2:56 AM Amit Kapila <amit.kapil...@gmail.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
v12-0001-Skip-logical-decoding-of-already-aborted-transac.patch
Description: Binary data
