Re: Skip collecting decoded changes of already-aborted transactions - Mailing list pgsql-hackers

On Tue, Jan 28, 2025 at 4:31 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sun, Jan 26, 2025 at 10:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jan 24, 2025 at 12:38 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Wed, Jan 22, 2025 at 7:35 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> > > > On Thu, Jan 23, 2025 at 2:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > On Wed, Jan 22, 2025 at 9:21 AM Peter Smith <smithpb2250@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)

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.

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

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

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

But if it does need to be assigned here, then why are there not the
same assertions about existing IS_PREPARED, SKIPPED and SENT as they
were in the other place where this flag was set?

======
Kind Regards,
Peter Smith.
Fujitsu Australia.