Re: Skip collecting decoded changes of already-aborted transactions

On Tue, Jan 7, 2025 at 7:22 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> ======
> src/include/replication/reorderbuffer.h
>
> 6.
> #define RBTXN_PREPARE 0x0040
> #define RBTXN_SKIPPED_PREPARE 0x0080
> #define RBTXN_HAS_STREAMABLE_CHANGE 0x0100
> +#define RBTXN_SENT_PREPARE 0x0200
> +#define RBTXN_IS_COMMITTED 0x0400
> +#define RBTXN_IS_ABORTED 0x0800
>
> Something about this new RBTXN_SENT_PREPARE name seems inconsistent to me.
>
> I feel there is now also some introduced ambiguity with these macros:
>
> /* Has this transaction been prepared? */
> #define rbtxn_prepared(txn) \
> ( \
> ((txn)->txn_flags & RBTXN_PREPARE) != 0 \
> )
>
> +/* Has a prepare or stream_prepare already been sent? */
> +#define rbtxn_sent_prepare(txn) \
> +( \
> + ((txn)->txn_flags & RBTXN_SENT_PREPARE) != 0 \
> +)
>
>
> e.g. It's also not clear from the comments what is the distinction
> between the existing macro comment "Has this transaction been
> prepared?" and the new macro comment "Has a prepare or stream_prepare
> already been sent?".
>
> Indeed, I was wondering if some of the places currently calling
> "rbtxn_prepared(txn)" should now strictly be calling
> "rbtxn_sent_prepared(txn)" macro instead?
>

Right, I think after this change, it appears we should try to rename
the existing constants. One place where we can consider to use new
macro is the current usage of rbtxn_prepared() in
SnapBuildDistributeNewCatalogSnapshot().

> IMO some minor renaming of the existing constants (and also their
> associated macros) might help to make all this more coherent. For
> example, perhaps like:
>
> #define RBTXN_IS_PREPARE_NEEDED 0x0040
>

The other option could be RBTXN_IS_PREPARE_REQUESTED.

--
With Regards,
Amit Kapila.