On Mon, Jan 13, 2025 at 3:07 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Jan 7, 2025 at 7:22 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > ====== > > src/include/replication/reorderbuffer.h > > > > 6. > > #define RBTXN_PREPARE 0x0040 > > #define RBTXN_SKIPPED_PREPARE 0x0080 > > #define RBTXN_HAS_STREAMABLE_CHANGE 0x0100 > > +#define RBTXN_SENT_PREPARE 0x0200 > > +#define RBTXN_IS_COMMITTED 0x0400 > > +#define RBTXN_IS_ABORTED 0x0800 > > > > Something about this new RBTXN_SENT_PREPARE name seems inconsistent to me. > > > > I feel there is now also some introduced ambiguity with these macros: > > > > /* Has this transaction been prepared? */ > > #define rbtxn_prepared(txn) \ > > ( \ > > ((txn)->txn_flags & RBTXN_PREPARE) != 0 \ > > ) > > > > +/* Has a prepare or stream_prepare already been sent? */ > > +#define rbtxn_sent_prepare(txn) \ > > +( \ > > + ((txn)->txn_flags & RBTXN_SENT_PREPARE) != 0 \ > > +) > > > > > > e.g. It's also not clear from the comments what is the distinction > > between the existing macro comment "Has this transaction been > > prepared?" and the new macro comment "Has a prepare or stream_prepare > > already been sent?". > > > > Indeed, I was wondering if some of the places currently calling > > "rbtxn_prepared(txn)" should now strictly be calling > > "rbtxn_sent_prepared(txn)" macro instead? > > > > Right, I think after this change, it appears we should try to rename > the existing constants. One place where we can consider to use new > macro is the current usage of rbtxn_prepared() in > SnapBuildDistributeNewCatalogSnapshot().
I think that RBTXN_PREPARE would mean that the transaction needs to be prepared but it doesn't mean that a prepare or a stream_prepare has already been sent. And RBTXN_SENT_PREPARE adds some internal details about whether a prepare or a stream_prepare has actually been sent. IIUC RBTXN_SENT_PREPARE is used only in a short term in ReorderBufferPrepare(). So outside of reorderbuffer such as snapbuild.c doesn't need to care about the RBTXN_SENT_PREPARE. > > > IMO some minor renaming of the existing constants (and also their > > associated macros) might help to make all this more coherent. For > > example, perhaps like: > > > > #define RBTXN_IS_PREPARE_NEEDED 0x0040 > > > > The other option could be RBTXN_IS_PREPARE_REQUESTED. I'm a bit concerned that these names sound like a state that the transaction needs to be prepared but has not been done yet. But rbtxn_prepared() is widely used to check if the transaction is a prepared transaction regardless of a prepare or a stream_prepare actually being sent. How about RBTXN_IS_PREPARED_TXN and rbtxn_is_preapred_txn()? I think it would indicate well that the transaction needs to be processed as a prepared transaction. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
