Re: Skip collecting decoded changes of already-aborted transactions - Mailing list pgsql-hackers

On Thu, Dec 19, 2024 at 2:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Dec 19, 2024 at 7:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Dec 9, 2024 at 9:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Tue, Nov 26, 2024 at 3:03 AM Masahiko Sawada <sawada.mshk@gmail.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.
>

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.

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

Agreed.

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

Agreed.

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().

>
> >
> > >
> > > 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().

The idea of using RBTXN_SENT_PREPARE sounds good to me. I'll use it.

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

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com