Re: Skip collecting decoded changes of already-aborted transactions

On Sun, Jan 26, 2025 at 10:26 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Jan 24, 2025 at 12:38 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Jan 22, 2025 at 7:35 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Jan 23, 2025 at 2:17 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Wed, Jan 22, 2025 at 9:21 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > > >
> > > > >
> > > > > ======
> > > > > Commit message
> > > > >
> > > > > typo /RBTXN_IS_PREAPRE/RBTXN_IS_PREPARE/
> > > > >
> >
> > Will fix.
> >
> > > > >
> > > > > Also, this code (below) seems to be treating those macros as
> > > > > unrelated, but IIUC we know that rbtxn_skip_prepared(txn) is not
> > > > > possible unless rbtxn_is_prepared(txn) is true.
> > > > >
> > > > > - if (rbtxn_prepared(txn) || rbtxn_skip_prepared(txn))
> > > > > + if (rbtxn_is_prepared(txn) || rbtxn_skip_prepared(txn))
> > > > > continue;
> >
> > Right. We no longer need to check rbtxn_skip_prepared() here.
> >
> > > > >
> > > > > ~~
> > > > >
> > > > > Furthermore, if we cannot infer that RBTXN_SKIPPED_PREPARE *must* also
> > > > > be a prepared transaction, then why aren't the macros changed to match
> > > > > that interpretation?
> > > > >
> > > > > e.g.
> > > > >
> > > > > /* prepare for this transaction skipped? */
> > > > > #define rbtxn_skip_prepared(txn) \
> > > > > ( \
> > > > > ((txn)->txn_flags & RBTXN_IS_PREPARED != 0) && \
> > > > > ((txn)->txn_flags & RBTXN_SKIPPED_PREPARE != 0) \
> > > > > )
> > > > >
> > > > > /* Has a prepare or stream_prepare already been sent? */
> > > > > #define rbtxn_sent_prepare(txn) \
> > > > > ( \
> > > > > ((txn)->txn_flags & RBTXN_IS_PREPARED != 0) && \
> > > > > ((txn)->txn_flags & RBTXN_SENT_PREPARE != 0) \
> > > > > )
> > > > >
> > > > > ~~~
> > > > >
> > > > > I think a to fix all this might be to enforce the RBTXN_IS_PREPARED
> > > > > bitflag is set also for RBTXN_SKIPPED_PREPARE and RBTXN_SENT_PREPARE
> > > > > constants, removing the ambiguity about how exactly to interpret those
> > > > > two constants.
> > > > >
> > > > > e.g. something like
> > > > >
> > > > > #define RBTXN_IS_PREPARED 0x0040
> > > > > #define RBTXN_SKIPPED_PREPARE (0x0080 | RBTXN_IS_PREPARED)
> > > > > #define RBTXN_SENT_PREPARE (0x0200 | RBTXN_IS_PREPARED)
> > > > >
> > > >
> > > > I think the better way would be to ensure that where we set
> > > > RBTXN_SENT_PREPARE or RBTXN_SKIPPED_PREPARE, the transaction is a
> > > > prepared one (RBTXN_IS_PREPARED must be already set). It should be
> > > > already the case for RBTXN_SENT_PREPARE but we can ensure the same for
> > > > RBTXN_SKIPPED_PREPARE as well.
> >
> > Since the patch already does "txn->txn_flags |= (RBTXN_IS_PREPARED |
> > RBTXN_SKIPPED_PREPARE);", it's already ensured, no?
> >
>
> I mean to say that we add assert to ensure the same.
>
> > I think we need to add both flags in ReorderBufferSkipPrepare(),
> > because there is a case where a transaction might not be marked as
> > RBTXN_IS_PREPARED here.
> >
>
> Are you talking about the case when it is invoked from
> DecodePrepare()?

Yes. IIUC ReorderBufferSkipPrepare() is called only from DecodePrepare().

> I thought we would set the flag in that code path.

I agree that it makes sense to add the flag before calling
ReorderBufferSkipPrepare().

>
> > > >
> > > > Will that address your concern? Does anyone else have an opinion on this matter?
> > >
> > > Yes that would be OK, but should also add some clarifying comments in
> > > the "reorderbuffer.h" like:
> > >
> > > #define RBTXN_SKIPPED_PREPARE 0x0080 /* this flag can only be set
> > > for RBTXN_IS_PREPARED transactions */
> > > #define RBTXN_SENT_PREPARE 0x0200 /* this flag can only be set for
> > > RBTXN_IS_PREPARED transactions */
> >
> > I think the same is true for RBTXN_IS_SERIALIZED and
> > RBTXN_IS_SERIALIZED_CLEAR; RBTXN_IS_SERIALIZED_CLEAR can only be set
> > for RBTXN_IS_SERIALIZED transaction. Should we add some comments to
> > them too? But I'm concerned about having too much explanation if we
> > add descriptions to flags too while already having comments for
> > corresponding macros.
> >
>
> Yeah, I am fine either way especially, if we decide to add asserts for
> RBTXN_IS_PREPARED when we set those flags.
>
> > Another way to ensure that is to convert these macros to inline
> > functions and add an Assert() there, but it seems overkill.
> >
>
> True, but that would ensure, we won't make any coding mistakes which
> Peter wants to ensure by writing additional comments but asserting is
> probably a better way.

I've attached the updated patch. In the 0002 patch, I've marked the
transaction as a prepared transaction in
ReorderBufferRememberPrepareInfo() so that all prepared transactions
that have a ReordeBufferTXN entry at that time can be marked properly.
And I've put some Assertions to ensure that all prepared transaction
related flags have been set properly. Thoughts?

Nothing changed to the 0001 patch from the previous version.

Regards,

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