Horton

Thread

Commits

  1. Hardwire RI fast-path end-of-xact cleanup into xact.c

  1. In core use of RegisterXactCallback() and RegisterSubXactCallback()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-25T04:06:21Z

    Hi hackers,

while working on [1] I wanted to make use of RegisterSubXactCallback() and
realized that RegisterXactCallback() has this comment:

"
 * These functions are intended for use by dynamically loaded modules.
 * For built-in modules we generally just hardwire the appropriate calls
 * (mainly because it's easier to control the order that way, where needed).
"

So I thought of hardwiring the call directly in Start/Commit/AbortSubTransaction()
instead.

Then I realized that b7b27eb41a5 just made use of RegisterXactCallback() and
RegisterSubXactCallback(), so I'm wondering if it should hardwire the calls
instead?

Note that the other RegisterSubXactCallback() and RegisterXactCallback() look
legitimate to me (as in loadable modules).

[1]: https://www.postgresql.org/message-id/ajVD9TjerBLteNlx%40bdtpg

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  2. Re: In core use of RegisterXactCallback() and RegisterSubXactCallback()

    Amit Langote <amitlangote09@gmail.com> — 2026-06-25T05:25:41Z

    Hi Bertrand,

On Thu, Jun 25, 2026 at 1:06 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi hackers,
>
> while working on [1] I wanted to make use of RegisterSubXactCallback() and
> realized that RegisterXactCallback() has this comment:
>
> "
>  * These functions are intended for use by dynamically loaded modules.
>  * For built-in modules we generally just hardwire the appropriate calls
>  * (mainly because it's easier to control the order that way, where needed).
> "
>
> So I thought of hardwiring the call directly in Start/Commit/AbortSubTransaction()
> instead.
>
> Then I realized that b7b27eb41a5 just made use of RegisterXactCallback() and
> RegisterSubXactCallback(), so I'm wondering if it should hardwire the calls
> instead?
>
> Note that the other RegisterSubXactCallback() and RegisterXactCallback() look
> legitimate to me (as in loadable modules).

Thanks for flagging this.

Note the RegisterSubXactCallback() call from b7b27eb41a5 was already
removed by a later fix (4113873a) that confined fast-path batching to
the top transaction level, so only the RegisterXactCallback() remains.
You're right that the header comment points toward hardwiring for
built-in code. I took the shortcut of using Register* because the RI
fast-path callback has no ordering dependency on the other hard-wired
work in Commit/AbortTransaction(), but I agree it should follow the
convention. I'll work up a patch converting it to a hardwired
AtEOXact_RI() called from CommitTransaction(), PrepareTransaction()
and AbortTransaction(), and post it on this thread.

-- 
Thanks, Amit Langote

  3. Re: In core use of RegisterXactCallback() and RegisterSubXactCallback()

    Amit Langote <amitlangote09@gmail.com> — 2026-06-25T13:17:13Z

    On Thu, Jun 25, 2026 at 2:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jun 25, 2026 at 1:06 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Hi hackers,
> >
> > while working on [1] I wanted to make use of RegisterSubXactCallback() and
> > realized that RegisterXactCallback() has this comment:
> >
> > "
> >  * These functions are intended for use by dynamically loaded modules.
> >  * For built-in modules we generally just hardwire the appropriate calls
> >  * (mainly because it's easier to control the order that way, where needed).
> > "
> >
> > So I thought of hardwiring the call directly in Start/Commit/AbortSubTransaction()
> > instead.
> >
> > Then I realized that b7b27eb41a5 just made use of RegisterXactCallback() and
> > RegisterSubXactCallback(), so I'm wondering if it should hardwire the calls
> > instead?
> >
> > Note that the other RegisterSubXactCallback() and RegisterXactCallback() look
> > legitimate to me (as in loadable modules).
>
> Thanks for flagging this.
>
> Note the RegisterSubXactCallback() call from b7b27eb41a5 was already
> removed by a later fix (4113873a) that confined fast-path batching to
> the top transaction level, so only the RegisterXactCallback() remains.
> You're right that the header comment points toward hardwiring for
> built-in code. I took the shortcut of using Register* because the RI
> fast-path callback has no ordering dependency on the other hard-wired
> work in Commit/AbortTransaction(), but I agree it should follow the
> convention. I'll work up a patch converting it to a hardwired
> AtEOXact_RI() called from CommitTransaction(), PrepareTransaction()
> and AbortTransaction(), and post it on this thread.

Here is a patch to do so.

It's a mechanical conversion, except the new AtEOXact_RI() takes
isCommit and asserts that the fast-path cache was already torn down by
commit, with a WARNING in non-assert builds. A survivor would mean a
trigger batch went unflushed. On abort it's expected, so it just
resets.

-- 
Thanks, Amit Langote

  4. Re: In core use of RegisterXactCallback() and RegisterSubXactCallback()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-25T15:05:01Z

    Hi Amit,

On Thu, Jun 25, 2026 at 10:17:13PM +0900, Amit Langote wrote:
> On Thu, Jun 25, 2026 at 2:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > Note the RegisterSubXactCallback() call from b7b27eb41a5 was already
> > removed by a later fix (4113873a) that confined fast-path batching to
> > the top transaction level, so only the RegisterXactCallback() remains.
> > You're right that the header comment points toward hardwiring for
> > built-in code. I took the shortcut of using Register* because the RI
> > fast-path callback has no ordering dependency on the other hard-wired
> > work in Commit/AbortTransaction(), but I agree it should follow the
> > convention. I'll work up a patch converting it to a hardwired
> > AtEOXact_RI() called from CommitTransaction(), PrepareTransaction()
> > and AbortTransaction(), and post it on this thread.
> 
> Here is a patch to do so.

Thanks for looking at it!

> It's a mechanical conversion, except the new AtEOXact_RI() takes
> isCommit and asserts that the fast-path cache was already torn down by
> commit, with a WARNING in non-assert builds.

That makes sense to me.

Just one nit:

+ Assert for development builds and, since
+        * the transaction is already committed by now and FK checks may have been
+        * skipped, also warn in production builds.

s/development builds/assert-enabled builds/? That looks more common and "
development builds" would be introduced by this patch.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  5. Re: In core use of RegisterXactCallback() and RegisterSubXactCallback()

    Amit Langote <amitlangote09@gmail.com> — 2026-06-25T23:21:50Z

    On Fri, Jun 26, 2026 at 12:05 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> On Thu, Jun 25, 2026 at 10:17:13PM +0900, Amit Langote wrote:
> > On Thu, Jun 25, 2026 at 2:25 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > >
> > > Note the RegisterSubXactCallback() call from b7b27eb41a5 was already
> > > removed by a later fix (4113873a) that confined fast-path batching to
> > > the top transaction level, so only the RegisterXactCallback() remains.
> > > You're right that the header comment points toward hardwiring for
> > > built-in code. I took the shortcut of using Register* because the RI
> > > fast-path callback has no ordering dependency on the other hard-wired
> > > work in Commit/AbortTransaction(), but I agree it should follow the
> > > convention. I'll work up a patch converting it to a hardwired
> > > AtEOXact_RI() called from CommitTransaction(), PrepareTransaction()
> > > and AbortTransaction(), and post it on this thread.
> >
> > Here is a patch to do so.
>
> Thanks for looking at it!
>
> > It's a mechanical conversion, except the new AtEOXact_RI() takes
> > isCommit and asserts that the fast-path cache was already torn down by
> > commit, with a WARNING in non-assert builds.
>
> That makes sense to me.

Thanks for checking.

> Just one nit:
>
> + Assert for development builds and, since
> +        * the transaction is already committed by now and FK checks may have been
> +        * skipped, also warn in production builds.
>
> s/development builds/assert-enabled builds/? That looks more common and "
> development builds" would be introduced by this patch.

I grepped, and while the word "development" isn't rare in the tree,
none of the uses are "development builds" as a build type.
"assert-enabled" is the established term for that, so I've switched to
it in v2.


--
Thanks, Amit Langote

  6. Re: In core use of RegisterXactCallback() and RegisterSubXactCallback()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-26T04:10:57Z

    Hi,

On Fri, Jun 26, 2026 at 08:21:50AM +0900, Amit Langote wrote:
> On Fri, Jun 26, 2026 at 12:05 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > Just one nit:
> >
> > + Assert for development builds and, since
> > +        * the transaction is already committed by now and FK checks may have been
> > +        * skipped, also warn in production builds.
> >
> > s/development builds/assert-enabled builds/? That looks more common and "
> > development builds" would be introduced by this patch.
> 
> I grepped, and while the word "development" isn't rare in the tree,
> none of the uses are "development builds" as a build type.

Right.

> "assert-enabled" is the established term for that, so I've switched to
> it in v2.

Thanks! LGTM.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  7. Re: In core use of RegisterXactCallback() and RegisterSubXactCallback()

    Amit Langote <amitlangote09@gmail.com> — 2026-06-29T01:26:33Z

    On Fri, Jun 26, 2026 at 1:11 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> On Fri, Jun 26, 2026 at 08:21:50AM +0900, Amit Langote wrote:
> > On Fri, Jun 26, 2026 at 12:05 AM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > > Just one nit:
> > >
> > > + Assert for development builds and, since
> > > +        * the transaction is already committed by now and FK checks may have been
> > > +        * skipped, also warn in production builds.
> > >
> > > s/development builds/assert-enabled builds/? That looks more common and "
> > > development builds" would be introduced by this patch.
> >
> > I grepped, and while the word "development" isn't rare in the tree,
> > none of the uses are "development builds" as a build type.
>
> Right.
>
> > "assert-enabled" is the established term for that, so I've switched to
> > it in v2.
>
> Thanks! LGTM.

Thanks, pushed.

-- 
Thanks, Amit Langote