Re: Skip collecting decoded changes of already-aborted transactions

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.

> ~
>
> 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.

--
With Regards,
Amit Kapila.