Re: Skip collecting decoded changes of already-aborted transactions

On Wed, Jan 22, 2025 at 9:21 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Jan 22, 2025 at 5:36 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Sun, Jan 19, 2025 at 7:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Jan 17, 2025 at 11:19 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > On Wed, Jan 15, 2025 at 4:43 PM Peter Smith <smithpb2250(at)gmail(dot)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)
>

I think the better way would be to ensure that where we set
RBTXN_SENT_PREPARE or RBTXN_SKIPPED_PREPARE, the transaction is a
prepared one (RBTXN_IS_PREPARED must be already set). It should be
already the case for RBTXN_SENT_PREPARE but we can ensure the same for
RBTXN_SKIPPED_PREPARE as well.

Will that address your concern? Does anyone else have an opinion on this matter?

--
With Regards,
Amit Kapila.