Re: Skip collecting decoded changes of already-aborted transactions

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

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

Another way to ensure that is to convert these macros to inline
functions and add an Assert() there, but it seems overkill.

Regards,

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