Re: Skip collecting decoded changes of already-aborted transactions

On Thu, Dec 19, 2024 at 7:14 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Dec 9, 2024 at 9:09 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Nov 26, 2024 at 3:03 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.
> > >
> >
> > Review comments:
>
> Thank you for reviewing the patch!
>
> > ===============
> > 1.
> > + * The given transaction is marked as streamed if appropriate and the caller
> > + * requested it by passing 'mark_txn_streaming' as true.
> > + *
> > * 'txn_prepared' indicates that we have decoded the transaction at prepare
> > * time.
> > */
> > static void
> > -ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
> > bool txn_prepared)
> > +ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
> > bool txn_prepared,
> > + bool mark_txn_streaming)
> > {
> > ...
> > }
> > + else if (mark_txn_streaming && (rbtxn_is_toptxn(txn) ||
> > (txn->nentries_mem != 0)))
> > + {
> > + /*
> > + * Mark the transaction as streamed, if appropriate.
> >
> > The comments related to the above changes don't clarify in which cases
> > the 'mark_txn_streaming' should be set. Before this patch, it was
> > clear from the comments and code about the cases where we would decide
> > to mark it as streamed.
>
> I think we can rename it to txn_streaming for consistency with
> txn_prepared. I've changed the comment for that.
>

@@ -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.

I think for the first case, we should get the streaming parameter in
ReorderBufferResetTXN(), and for the second case
ReorderBufferStreamCommit(), we should pass it as false because by
that time transaction is already streamed and prepared. We are
invoking it for cleanup. Even when we call ReorderBufferTruncateTXN()
from ReorderBufferCheckAndTruncateAbortedTXN(), it will be better to
write a comment at the caller about why we are passing this parameter
as false.

>
> >
> > 4.
> > + /*
> > + * Remember if the transaction is already aborted so we can detect when
> > + * the transaction is concurrently aborted during the replay.
> > + */
> > + already_aborted = rbtxn_is_aborted(txn);
> > +
> > ReorderBufferReplay(txn, rb, xid, txn->final_lsn, txn->end_lsn,
> > txn->xact_time.prepare_time, txn->origin_id, txn->origin_lsn);
> >
> > @@ -2832,10 +2918,10 @@ ReorderBufferPrepare(ReorderBuffer *rb,
> > TransactionId xid,
> > * when rollback prepared is decoded and sent, the downstream should be
> > * able to rollback such a xact. See comments atop DecodePrepare.
> > *
> > - * Note, for the concurrent_abort + streaming case a stream_prepare was
> > + * Note, for the concurrent abort + streaming case a stream_prepare was
> > * already sent within the ReorderBufferReplay call above.
> > */
> > - if (txn->concurrent_abort && !rbtxn_is_streamed(txn))
> > + if (!already_aborted && rbtxn_is_aborted(txn) && !rbtxn_is_streamed(txn))
> > rb->prepare(rb, txn, txn->final_lsn);
> >
> > It is not clear from the comments how the 'already_aborted' is
> > handled. I think after this patch we would have already truncated all
> > its changes. If so, why do we need to try to replay the changes of
> > such a xact?
>
> I used ReorderBufferReplay() for convenience; it sends begin_prepare()
> and prepare() appropriately, handles streaming-prepared transactions,
> and updates statistics etc. But as you pointed out, it would not be
> necessary to set up a historical snapshot etc. I agree that we don't
> need to try replaying such aborted transactions but I'd like to
> confirm we don't really need to execute invalidation messages evein in
> aborted transactions.
>

We need to execute invalidations if we have loaded any cache entries,
for example in the case of streaming. See comments in the function
ReorderBufferAbort(). However, I find both the current changes and the
previous patch a bit difficult to follow. How about if we instead
invent a flag like RBTXN_SENT_PREPARE or something like that and then
use that flag to decide whether to send prepare in
ReorderBufferPrepare(). Then add comments for the cases in which
prepare will be sent from ReorderBufferPrepare().

*
+ * 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?

--
With Regards,
Amit Kapila.