Re: Skip collecting decoded changes of already-aborted transactions

On Mon, Jan 27, 2025 at 7:01 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Tue, Jan 28, 2025 at 4:31 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > 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.
>
> Hm That RBTXN_IS_SERIALIZED / RBTXN_IS_SERIALIZED_CLEAR is used
> differently -- it seems more tricky because RBTXN_IS_SERIALIZED flag
> is turned OFF again when RBTXN_IS_SERIALIZED_CLEAR is turned ON.
> (Whereas setting SKIPPED_PREPARE and SENT_PREPARE will never turn off
> the tx type IS_PREPARED)

You're right.

> To be honest, I didn't understand the "CLEAR" part of that name. It
> seems more like it should've been called something like
> RBTXN_IS_SERIALIZED_ALREADY or RBTXN_IS_SERIALIZED_PREVIOUSLY or
> whatever instead of something that appears to be saying "has the
> RBTXN_IS_SERIALIZED bitflag been cleared?" I understand the reluctance
> to over-comment everything but OTOH currently there is no way really
> to understand what these flags mean without looking through all the
> code to try to figure them out from the usage.
>
> My recurring gripe about these flags is simply that their meanings and
> how to use them should be apparent just by looking at reorderbuffer.h
> and not having to guess anything or look at how they get used in the
> code. It doesn't matter if that is achieved by better constant names,
> by more comments or by enhanced macros/functions with asserts but
> currently just looking at that file still leaves the reader with lots
> of unanswered questions.

I see your point. IIUC we have the comments about what the checks with
the flags means but not have the description about the relationship
among the flags. I think we can start a new thread for clarifying
these flags and their usage. We can also discuss renaming
RBTXN_IS_SERIALIZED[_CLEARE] there too.

>
> > > >
> > >
> > > 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.
> >
>
> Maybe I misunderstood, but I thought Amit's reply there meant that
> rewriting the macros as inline functions with asserts would be a good
> way to ensure no coding mistakes. Yet, the macros are still unchanged
> in v16-0002.

I forgot to mention; while converting all macros to inline functions
is a good idea, adding assertions to some places reasonably also makes
the code robust. The prepared transactions related flags are currently
used in specific cases. So I thought what the patch does also makes
sense to me.

>
> > 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?
> >
>
> Here are a couple of other review comments for patch v16-0002

Thank you for the comments!

>
> ======
> Commit message
>
> 1.
> The RBTXN_PREPARE flag (and its corresponding macro) have been renamed
> to RBTXN_IS_PREPARE to explicitly indicate the transaction
> type. Therefore, this commit also adds the RBTXN_IS_PREAPRE flag also
> to the transaction that is a prepared transaction and has been
> skipped, which previously had only the RBTXN_SKIPPED_PREPARE flag.
>
> Instead of fixing the "RBTXN_IS_PREAPRE" typo, it looks like a new
> problem (double "also" in the same sentence) was added in v16.

Fixed.

>
> ======
> .../replication/logical/reorderbuffer.c
>
> 2.
> if ((txn->final_lsn < two_phase_at) && is_commit)
> {
> - txn->txn_flags |= RBTXN_PREPARE;
> + txn->txn_flags |= RBTXN_IS_PREPARED;
>
> Won't this flag be already this flag already set? The next code
> comment ("The prepare info must have been updated ...") made me think
> so.
>

Good point. The transaction must have both flags: RBTXN_IS_PREPARED
and RBTXN_SKIPPED_PREPARE, unless I'm missing something.

I've attached the updated patches.

BTW if there is no comment on 0001 patch, I'm going to push it this week .

Regards,

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