Re: Skip collecting decoded changes of already-aborted transactions - Mailing list pgsql-hackers

On Thu, Jan 30, 2025 at 7:07 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Jan 31, 2025 at 11:04 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Jan 29, 2025 at 9:32 PM Peter Smith <smithpb2250@gmail.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'm not sure if ReorderBufferCheckMemoryLimit() is an unrelated
function because the whole idea (also mentioned in the commit message)
is that we check the transaction status only for large transactions to
avoid CLOG lookup overheads. TBH I'm not sure why readers expect these
transaction status flags to always be set. Also in the function
comment we have:

 * the transaction is aborted. The transaction status is cached in
 * txn->txn_flags so we can skip future changes and avoid CLOG lookups on the
 * next call.

which describes the side-effect of the function that it caches the
transaction status.

> I don't know what the solution is; maybe some
> more comments would be enough.

I'm not sure how we can improve the comment TBH.

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

Okay, addressed in the v18 patch I've just sent[1].

Regards,

[1] https://www.postgresql.org/message-id/CAD21AoDmYZtLnPLuiERT6Cibv1Gf1DwDjzBevtqKYn0ZzMQqBQ%40mail.gmail.com

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