Re: Skip collecting decoded changes of already-aborted transactions - Mailing list pgsql-hackers
From | vignesh C |
---|---|
Subject | Re: Skip collecting decoded changes of already-aborted transactions |
Date | |
Msg-id | CALDaNm2A9sJzsqugorJap8Kd90FiGj69vdgPqr+7DPaXq9oxtQ@mail.gmail.com Whole thread Raw |
In response to | Re: Skip collecting decoded changes of already-aborted transactions (Masahiko Sawada <sawada.mshk@gmail.com>) |
Responses |
Re: Skip collecting decoded changes of already-aborted transactions
|
List | pgsql-hackers |
On Tue, 26 Nov 2024 at 02:58, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Mon, Nov 18, 2024 at 11:12 PM vignesh C <vignesh21@gmail.com> wrote: > > > > > > Few comments: > > Thank you for reviewing the patch! > > > 1) Should we have the Assert inside ReorderBufferTruncateTXNIfAborted > > instead of having it at multiple callers, ReorderBufferResetTXN also > > has the Assert inside the function after truncate of the transaction: > > @@ -3672,6 +3758,14 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb) > > Assert(txn->total_size > 0); > > Assert(rb->size >= txn->total_size); > > > > + /* skip the transaction if aborted */ > > + if (ReorderBufferTruncateTXNIfAborted(rb, txn)) > > + { > > + /* All changes should be discarded */ > > + Assert(txn->size == 0 && txn->total_size == 0); > > + continue; > > + } > > + > > ReorderBufferStreamTXN(rb, txn); > > } > > else > > @@ -3687,6 +3781,14 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb) > > Assert(txn->size > 0); > > Assert(rb->size >= txn->size); > > > > + /* skip the transaction if aborted */ > > + if (ReorderBufferTruncateTXNIfAborted(rb, txn)) > > + { > > + /* All changes should be discarded */ > > + Assert(txn->size == 0 && txn->total_size == 0); > > + continue; > > + } > > Moved. > > > > > 2) txn->txn_flags can be moved to the next line to keep it within 80 > > chars in this case: > > * Check the transaction status by looking CLOG and discard all changes if > > * the transaction is aborted. The transaction status is cached in > > txn->txn_flags > > * so we can skip future changes and avoid CLOG lookups on the next call. Return > > Fixed. > > > > > 3) Is there any scenario where the Assert can fail as the toast is not reset: > > + * 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); > > > > + if (ReorderBufferTruncateTXNIfAborted(rb, txn)) > > + { > > + /* All changes should be discarded */ > > + Assert(txn->size == 0 && txn->total_size == 0); > > + continue; > > + } > > IIUC we reconstruct TOAST data when replaying the transaction. On the > other hand, this function is called while adding a decoded change but > not when replaying the transaction. So we should not have any toast > reconstruction data at this point unless I'm missing something. Do you > have any scenario where we call ReorderBufferTruncateTXNIfAborted() > while a transaction has TOAST reconstruction data? I have checked further regarding the toast and verified the population of the toast hash. I agree with you on this. Overall, the patch appears to be in good shape. Regards, Vignesh
pgsql-hackers by date: