On Mon, Jan 13, 2025 at 8:48 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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.
I agree with the above usage; it's more consistent to set RBTXN_IS_PREPARED also for a skipped prepared transaction. But I'm not sure it's better to have the RBTXN_IS_PREPARED prefix for all constants. > > ~ > > > > 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. I followed RBTXN_IS_SUBXACT (I think TXN and XACT have the same meaning) but that's a fair point. It seems we agreed on RBTXN_IS_PREPARED and rbtxn_is_prepared(). Adding 'IS' seems to clarify the transaction having this flag *is* a prepared transaction. Both other two constants RBTXN_SENT_PREAPRE and RBTXN_SKIPPED_PREPARE seem not bad to me. I find that the proposed names don't increase the consistency much. Thoughts? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
