In core use of RegisterXactCallback() and RegisterSubXactCallback()

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

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

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

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

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