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(). > 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. -- With Regards, Amit Kapila.
