Re: Skip collecting decoded changes of already-aborted transactions

On Mon, Jan 13, 2025 at 3:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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().

I think that RBTXN_PREPARE would mean that the transaction needs to be
prepared but it doesn't mean that a prepare or a stream_prepare has
already been sent. And RBTXN_SENT_PREPARE adds some internal details
about whether a prepare or a stream_prepare has actually been sent.
IIUC RBTXN_SENT_PREPARE is used only in a short term in
ReorderBufferPrepare(). So outside of reorderbuffer such as
snapbuild.c doesn't need to care about the RBTXN_SENT_PREPARE.

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

I'm a bit concerned that these names sound like a state that the
transaction needs to be prepared but has not been done yet. But
rbtxn_prepared() is widely used to check if the transaction is a
prepared transaction regardless of a prepare or a stream_prepare
actually being sent. How about RBTXN_IS_PREPARED_TXN and
rbtxn_is_preapred_txn()? I think it would indicate well that the
transaction needs to be processed as a prepared transaction.

Regards,

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