On Mon, Jan 27, 2025 at 7:01 PM Peter Smith <smithpb2...@gmail.com> wrote: > > On Tue, Jan 28, 2025 at 4:31 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Sun, Jan 26, 2025 at 10:26 PM Amit Kapila <amit.kapil...@gmail.com> > > wrote: > > > > > > On Fri, Jan 24, 2025 at 12:38 AM Masahiko Sawada <sawada.m...@gmail.com> > > > wrote: > > > > > > > > On Wed, Jan 22, 2025 at 7:35 PM Peter Smith <smithpb2...@gmail.com> > > > > wrote: > > > > > > > > > > On Thu, Jan 23, 2025 at 2:17 PM Amit Kapila <amit.kapil...@gmail.com> > > > > > wrote: > > > > > > > > > > > > On Wed, Jan 22, 2025 at 9:21 AM Peter Smith <smithpb2...@gmail.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
v17-0001-Skip-logical-decoding-of-already-aborted-transac.patch
Description: Binary data
v17-0002-Rename-RBTXN_PREPARE-to-RBTXN_IS_PREPARE-for-bet.patch
Description: Binary data
