Re: Skip collecting decoded changes of already-aborted transactions

On Fri, Jan 31, 2025 at 11:04 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Jan 29, 2025 at 9:32 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > ======
> > .../replication/logical/reorderbuffer.c
> >
> > ReorderBufferCheckAndTruncateAbortedTXN:
> >
> > 2.
> > It seemed tricky that the only place that is setting the
> > RBTXN_IS_COMMITTED flag is the function
> > ReorderBufferCheckAndTruncateAbortedTXN because neither the function
> > name nor the function comment gives any indication that it should be
> > having this side effect
>
> Hmm, it doesn't seem so tricky to me that a function with the name
> ReorderBufferCheckAndTruncateAbortedTXN() checks the transaction
> status to truncate an aborted transaction and caches the transaction
> status as a side effect.
>

I was coming at this from a different perspective, asking myself the
question "When can I know the RBTXN_IS_COMMITTED bit setting?" -- aka
rbtxn_is_committed()?

AFAICT it turns out we can only have confidence in that result when
know ReorderBufferCheckAndTruncateAbortedTXN was called already for
this tx. But this happens only when ReorderBufferCheckMemoryLimit()
gets called. So, these bitflags are getting set as a side-effect of
calling unrelated functions. (e.g. the fact we can't test if a tx was
aborted/committed unless ReorderBufferCheckMemoryLimit is called
seemed unusual to me). I don't know what the solution is; maybe some
more comments would be enough.

> >
> > ~~~
> >
> > ReorderBufferProcessTXN:
> >
> > 3.
> > if (rbtxn_prepared(txn))
> > + {
> > rb->prepare(rb, txn, commit_lsn);
> > + txn->txn_flags |= RBTXN_SENT_PREPARE;
> > + }
> >
> > In ReorderBufferStreamCommit there is an assertion that we are not
> > trying to do another prepare() if the _SENT_PREPARE flag is already
> > set. Should this code have a similar assert?
>
> We can have a similar assert there but why do you think it's needed there?

No particular reason, other than for consistency to have similar
assertions everywhere that the RBTXN_SENT_PREPARE flag is set.

>
> >
> > ======
> > src/include/replication/reorderbuffer.h
> >
> > 4.
> > +#define RBTXN_SENT_PREPARE 0x0200
> > +#define RBTXN_IS_COMMITTED 0x0400
> > +#define RBTXN_IS_ABORTED 0x0800
> >
> > IIUC, unlike the _SENT_PREPARE, those _IS_COMMITTED and _IS_ABORTED
> > flags are not quite the same as saying rb->commit() or rb->abort() was
> > called. But, those flags are only set some time later by
> > ReorderBufferCheckAndTruncateAbortedTXN() function based on the commit
> > log status.
> >
> > The lag between the commit/abort happening and these flag getting set
> > seems unintuitive. Should they be named differently -- e.g. maybe
> > RBTXN_IS_CLOG_COMMITTED, RBTXN_IS_CLOG_ABORTED instead?
> >
>
> I'm not sure these names are better.
>
> In logical decoding context, we neither commit nor rollback
> transactions decoded from WAL records as the transaction outcomes come
> only from WAL records. So I guess it's easy-to-grasp that
> RBTXN_IS_COMMITTED means "this is a committed transaction" but not "we
> committed the transaction". I think this is a similar understanding as
> what we're trying to rename RBTXN_PREPARE to RBTXN_IS_PREPARE.
> Similarly, we have rb->commit() and rb->abort(), I would not think
> like we're committing or aborting the transaction. So the lag between
> the ->commit()/abort() happening and these flags getting set is not
> confusing (at least for me). I think we can leave these names as they
> are, and if we need to remember if a commit message has been sent, we
> would be able to have a flag like RBTXN_SENT_COMMIT.
>

OK.

======
Kind Regards,
Peter Smith.
Fujitsu Australia