Re: Skip collecting decoded changes of already-aborted transactions

On Mon, Jan 13, 2025 at 8:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jan 14, 2025 at 7:32 AM Peter Smith <smithpb2250(at)gmail(dot)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