Re: Skip collecting decoded changes of already-aborted transactions

On Wed, Jan 15, 2025 at 4:43 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Jan 15, 2025 at 5:49 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Jan 15, 2025 at 3:11 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > 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.
> > >
> >
> > Agreed.
> >
> > > I find that the proposed
> > > names don't increase the consistency much. Thoughts?
> > >
> >
> > I also think so.
> >
>
> 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.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com