Re: Skip collecting decoded changes of already-aborted transactions - Mailing list pgsql-hackers
From | Amit Kapila |
---|---|
Subject | Re: Skip collecting decoded changes of already-aborted transactions |
Date | |
Msg-id | CAA4eK1Ljpx3qsfF6_YxsqDqNEp7hMcc0QxFoqFa1_sfsC7zK6g@mail.gmail.com Whole thread Raw |
In response to | Skip collecting decoded changes of already-aborted transactions (Masahiko Sawada <sawada.mshk@gmail.com>) |
List | pgsql-hackers |
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()? 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.
pgsql-hackers by date: