Re: Skip collecting decoded changes of already-aborted transactions

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()? I thought we would set the flag in that code path.

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

--
With Regards,
Amit Kapila.