On Tue, Jan 14, 2025 at 7:32 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Hi Sawada-San. > > Some review comments for patch v13-0002. > > ====== > > I think the v12 ambiguity of RBTXN_PREPARE versus RBTXN_SENT_PREPARE > was mostly addressed already by the improved comments for the macros > in patch 0001. > > Meanwhile, patch v13-0002 says it is renaming constants for better > consistency, but I don't think it went far enough. > > For example, better name consistency would be achieved by changing > *all* of the constants related to prepared transactions: > > #define RBTXN_IS_PREPARED 0x0040 > #define RBTXN_IS_PREPARED_SKIPPED 0x0080 > #define RBTXN_IS_PREPARED_SENT 0x0200 > > where: > > RBTXN_IS_PREPARED. This means it's a prepared transaction. (but we > can't tell from this if it is skipped or sent). > > RBTXN_IS_PREPARED_SKIPPED. This means it's a prepared transaction > (RBTXN_IS_PREPARED) and it's being skipped. > > RBTXN_IS_PREPARED_SENT. This means it's a prepared transaction > (RBTXN_IS_PREPARED) and we've sent it. >
The first one (RBTXN_IS_PREPARED) sounds like an improvement over what we have now. I am not convinced about the other two. > ~ > > A note about RBTXN_IS_PREPARED. Since all of these constants are > clearly about transactions (e.g. "TXN" in prefix "RBTXN_"), I felt > patch 0002 calling this RBTXN_IS_PREPARED_TXN just seemed like adding > a redundant _TXN. e.g. we don't say RBTXN_IS_COMMITTED_TXN etc. > +1. I felt the same. -- With Regards, Amit Kapila.
