On Wed, Jan 22, 2025 at 5:36 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Sun, Jan 19, 2025 at 7:53 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Fri, Jan 17, 2025 at 11:19 PM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > On Wed, Jan 15, 2025 at 4:43 PM Peter Smith <smithpb2...@gmail.com> wrote: > > > > > > > > My thoughts are that any consistency improvement is a step in the > > > > right direction so even "don't increase the consistency much" is still > > > > better than nothing. > > > > > > I agree that doing something is better than nothing. The proposed > > > idea, having RBTXN_IS_PREPARED prefix for all related flags, improves > > > the consistency in terms of names, but I'm not sure this is the right > > > direction. For example, RBTXN_IS_PREPARED_SKIPPED is quite confusing > > > to me. I think this name implies "this is a prepared transaction but > > > is skipped", but I don't think it conveys the meaning well. In > > > addition to that, if we add RBTXN_IS_PREPARED flag also for skipped > > > prepared transactions, we would end up with doing like: > > > > > > txn->txn_flags |= (RBTXN_IS_PREPARED | RBTXN_IS_PREPARED_SKIPPED); > > > > > > Which seems quite redundant. It makes more sense to me to do like: > > > > > > txn->txn_flags |= (RBTXN_IS_PREPARED | RBTXN_SKIPPED_PREPARE); > > > > > > I'd like to avoid a situation like where we rename these names just > > > for better consistency in terms of names and later rename them to > > > better names for other reasons again and again. > > > > > > > Sounds reasonable. We agree with just changing RBTXN_PREPARE to > > RBTXN_IS_PREPARED and its corresponding macro. The next step is to > > update the patch to reflect the same. > > Right. I've attached the updated patches. >
Some review comments for v15-0002. ====== Commit message typo /RBTXN_IS_PREAPRE/RBTXN_IS_PREPARE/ ====== I'm not trying to be pedantic, but there seems to be something strange about the combination usage of these PREPARE constants, which raises lots of questions for me... For example. I had thought RBTXN_SKIPPED_PREPARE meant it is a prepared tx AND it is skipped I had thought RBTXN_SENT_PREPARE meant it is a prepared tx AND it is sent So I was surprised that the patch makes this change: - txn->txn_flags |= RBTXN_SKIPPED_PREPARE; + txn->txn_flags |= (RBTXN_IS_PREPARED | RBTXN_SKIPPED_PREPARE); because, if we cannot infer that RBTXN_SKIPPED_PREPARE *must* mean it is a prepared transaction then why does that constant even have "PREPARE" in its name at all instead of just being called RBTXN_SKIPPED? e.g., either of these makes sense to me: txn->txn_flags |= (RBTXN_IS_PREPARED | RBTXN_SKIPPED); txn->txn_flags |= RBTXN_SKIPPED_PREPARE; But this combination seemed odd: txn->txn_flags |= (RBTXN_IS_PREPARED | RBTXN_SKIPPED_PREPARE); 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; ~~ 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) and make appropriate macro changes e.g. /* prepare for this transaction skipped? */ #define rbtxn_skip_prepared(txn) \ ( \ ((txn)->txn_flags & RBTXN_SKIPPED_PREPARE == RBTXN_SKIPPED_PREPARE) \ ) /* Has a prepare or stream_prepare already been sent? */ #define rbtxn_sent_prepare(txn) \ ( \ ((txn)->txn_flags & RBTXN_SENT_PREPARE == RBTXN_SENT_PREPARE) \ ) Thoughts? ====== Kind Regards, Peter Smith. Fujitsu Australia