[PATCH] refint: Avoid reusing cascade UPDATE plans.

On Fri, May 15, 2026 at 12:42:18AM +0530, Ayush Tiwari wrote:

I had originally tried parameterizing the SET values, but as Nathan
pointed out in the BUG #19476 thread, that is not generally correct
because the referenced-relation key type is not necessarily the right
type for the referencing relation's SET target. This patch instead
uses the simpler approach suggested there: do not cache prepared plans
for cascade UPDATE actions. The existing cached-plan path remains in
place for restrict, cascade DELETE, and setnull actions.

This looks generally reasonable. My first thought was that we would leave
most of check_foreign_key() the same, but we'd evict the plans at the end.
I am not sure that's any particular advantage to that approach, and
skipping the extra work for a temporary plan is arguably better.

That being said, the plan cache still has problems. There's no
invalidation mechanism, so you could still end up with the wrong plan.
Here's a quick example:

CREATE EXTENSION refint;

CREATE TABLE p AS SELECT 1 AS a;
CREATE TABLE f1 AS SELECT 1 AS a;
CREATE TABLE f2 AS SELECT 1 AS a;
CREATE TRIGGER t
AFTER DELETE OR UPDATE ON p
FOR EACH ROW EXECUTE PROCEDURE
check_foreign_key(2, 'c', 'a', 'f1', 'a', 'f2', 'a');
UPDATE p SET a = 2;

DROP TRIGGER t ON p;
CREATE TRIGGER t
AFTER DELETE OR UPDATE ON p
FOR EACH ROW EXECUTE PROCEDURE
check_foreign_key(1, 'c', 'a', 'f1', 'a');
UPDATE p SET a = 3;

The last UPDATE fails with:

ERROR: t: check_foreign_key: # of plans changed in meantime

A simple way to fix this could be to use the trigger OID instead of the
trigger name in the plan cache key. That's not perfect because the OID
could be reused, but IMHO it's better than what's there today. An even
better approach would involve more sophisticated invalidation or removing
the cache altogether.

--
nathan

Hi,

On Fri, 15 May 2026 at 02:44, Nathan Bossart <nathandbossart@gmail.com>
wrote:

That being said, the plan cache still has problems. There's no
invalidation mechanism, so you could still end up with the wrong plan.
Here's a quick example:

CREATE EXTENSION refint;

CREATE TABLE p AS SELECT 1 AS a;
CREATE TABLE f1 AS SELECT 1 AS a;
CREATE TABLE f2 AS SELECT 1 AS a;
CREATE TRIGGER t
AFTER DELETE OR UPDATE ON p
FOR EACH ROW EXECUTE PROCEDURE
check_foreign_key(2, 'c', 'a', 'f1', 'a', 'f2', 'a');
UPDATE p SET a = 2;

DROP TRIGGER t ON p;
CREATE TRIGGER t
AFTER DELETE OR UPDATE ON p
FOR EACH ROW EXECUTE PROCEDURE
check_foreign_key(1, 'c', 'a', 'f1', 'a');
UPDATE p SET a = 3;

The last UPDATE fails with:

ERROR: t: check_foreign_key: # of plans changed in meantime

A simple way to fix this could be to use the trigger OID instead of the
trigger name in the plan cache key. That's not perfect because the OID
could be reused, but IMHO it's better than what's there today. An even
better approach would involve more sophisticated invalidation or removing
the cache altogether.

Thanks for the review and the drop/recreate example.

After looking at it again, the root issue is that check_foreign_key()'s
private plan cache is not robust enough for the SQL it stores: the
cascade UPDATE query text embeds the current NEW key values in its SET
clause, and the cache itself has no invalidation mechanism, so the
trigger-name-keyed entries survive a drop and recreate. Rather than
keep teaching the cache more special cases, v2 just removes the cache
from check_foreign_key() entirely. Plans are prepared on each trigger
invocation and released by SPI_finish(). refint is example code, so I
think the simplicity is worth more than the per-call SPI_prepare.

check_primary_key() still uses the existing per-trigger cache; its
generated SELECT does not embed row values and the same invalidation
concern is far less interesting there, so I left it alone for now.

v2 keeps the cascade-UPDATE regression test from v1 and adds a second
test that drops and recreates a cascade trigger with a different number
of references, which reproduces your example.

Regards,
Ayush

On Fri, May 15, 2026 at 12:13:21PM +0530, Ayush Tiwari wrote:

After looking at it again, the root issue is that check_foreign_key()'s
private plan cache is not robust enough for the SQL it stores: the
cascade UPDATE query text embeds the current NEW key values in its SET
clause, and the cache itself has no invalidation mechanism, so the
trigger-name-keyed entries survive a drop and recreate. Rather than
keep teaching the cache more special cases, v2 just removes the cache
from check_foreign_key() entirely. Plans are prepared on each trigger
invocation and released by SPI_finish(). refint is example code, so I
think the simplicity is worth more than the per-call SPI_prepare.

Yeah, this may be the best option. I'm curious what others think. My
other idea was to use the trigger OID instead of the name as the key, but
that doesn't handle OID reuse. Given this is sample code, that also risks
propagation to third-party code.

check_primary_key() still uses the existing per-trigger cache; its
generated SELECT does not embed row values and the same invalidation
concern is far less interesting there, so I left it alone for now.

Huh? Why is it less interesting? Is it not subject to similar bugs? If
we're going to remove the cache, I think we ought to remove it completely.

--
nathan

On 2026-May-15, Nathan Bossart wrote:

Yeah, this may be the best option. I'm curious what others think.

I think this bug makes it clear that this code has few enough users, and
a sufficient number of bugs, that we should just nuke it. Do you really
want to spend so much time trying to fix it? The module's documentation
says

(This functionality is long since superseded by the built-in foreign
key mechanism, of course, but the module is still useful as an example.)

but I question whether the usefulness value is really there.

Would anyone oppose that?

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"How amazing is that? I call it a night and come back to find that a bug has
been identified and patched while I sleep." (Robert Davidson)
http://archives.postgresql.org/pgsql-sql/2006-03/msg00378.php

On Fri, May 15, 2026 at 04:42:45PM +0200, Álvaro Herrera wrote:

On 2026-May-15, Nathan Bossart wrote:

Yeah, this may be the best option. I'm curious what others think.

I think this bug makes it clear that this code has few enough users, and
a sufficient number of bugs, that we should just nuke it. Do you really
want to spend so much time trying to fix it? The module's documentation
says

(This functionality is long since superseded by the built-in foreign
key mechanism, of course, but the module is still useful as an example.)

but I question whether the usefulness value is really there.

Would anyone oppose that?

Big +1 for removing it in v20. Or maybe even v19. I do think it is worth
trying to fix the more egregious bugs, though, as the code will still be
supported for a few years.

--
nathan

Hi,

On Fri, 15 May 2026 at 20:16, Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Fri, May 15, 2026 at 04:42:45PM +0200, Álvaro Herrera wrote:

On 2026-May-15, Nathan Bossart wrote:

Yeah, this may be the best option. I'm curious what others think.

I think this bug makes it clear that this code has few enough users, and
a sufficient number of bugs, that we should just nuke it. Do you really
want to spend so much time trying to fix it? The module's documentation
says

(This functionality is long since superseded by the built-in foreign
key mechanism, of course, but the module is still useful as an

example.)

but I question whether the usefulness value is really there.

Would anyone oppose that?

Big +1 for removing it in v20. Or maybe even v19. I do think it is worth
trying to fix the more egregious bugs, though, as the code will still be
supported for a few years.

I agree on the removal part.

In the meantime, since the module will still be
supported for a while, this seems like a focused fix for the more
egregious cache behavior.

Attached v3 removes the private SPI plan cache from refint entirely.
Both check_primary_key() and check_foreign_key() now prepare their SPI
plans per trigger invocation and let SPI_finish() release them.

Regards,
Ayush

On Fri, May 15, 2026 at 08:34:55PM +0530, Ayush Tiwari wrote:

On Fri, 15 May 2026 at 20:16, Nathan Bossart <nathandbossart@gmail.com>
wrote:

Big +1 for removing it in v20. Or maybe even v19. I do think it is worth
trying to fix the more egregious bugs, though, as the code will still be
supported for a few years.

I agree on the removal part.

Cool. Do you want to write the patch for that, too? I think we should aim
for removing it shortly after v20 opens for development. It might be best
to move that to its own thread so that folks are aware and have a chance to
object.

In the meantime, since the module will still be
supported for a while, this seems like a focused fix for the more
egregious cache behavior.

Attached v3 removes the private SPI plan cache from refint entirely.
Both check_primary_key() and check_foreign_key() now prepare their SPI
plans per trigger invocation and let SPI_finish() release them.

I'm not sure I'd bother with the tests. At a glance the rest looks
generally reasonable. I realize that leaving the braces around the plan
preparation logic keeps the patch small, but we probably wouldn't write it
that way today. I'd suggest moving the local variable declarations to the
top of the function in your patch, and then doing a follow-up patch to
re-pgindent the file. Also, I don't think we need any of the comments
about the historical usage of a cache; let's just clean house.

--
nathan

Hi,

On Fri, 15 May 2026 at 22:57, Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Fri, May 15, 2026 at 08:34:55PM +0530, Ayush Tiwari wrote:

On Fri, 15 May 2026 at 20:16, Nathan Bossart <nathandbossart@gmail.com>
wrote:

Big +1 for removing it in v20. Or maybe even v19. I do think it is

worth

trying to fix the more egregious bugs, though, as the code will still be
supported for a few years.

I agree on the removal part.

Cool. Do you want to write the patch for that, too? I think we should aim
for removing it shortly after v20 opens for development. It might be best
to move that to its own thread so that folks are aware and have a chance to
object.

Yes, I would love to work on it. I'll start a new thread in a few days for
v20.

In the meantime, since the module will still be
supported for a while, this seems like a focused fix for the more
egregious cache behavior.

Attached v3 removes the private SPI plan cache from refint entirely.
Both check_primary_key() and check_foreign_key() now prepare their SPI
plans per trigger invocation and let SPI_finish() release them.

I'm not sure I'd bother with the tests. At a glance the rest looks
generally reasonable. I realize that leaving the braces around the plan
preparation logic keeps the patch small, but we probably wouldn't write it
that way today. I'd suggest moving the local variable declarations to the
top of the function in your patch, and then doing a follow-up patch to
re-pgindent the file. Also, I don't think we need any of the comments
about the historical usage of a cache; let's just clean house.

Thanks for looking. Attached v4 drops the regression-test additions,
removes the historical cache comments, and moves the new local variable
declarations to the tops of the functions.

I've split the pgindent result into a second patch.

Regards,
Ayush

Committed.

--
nathan

Hi,

On Fri, 5 Jun 2026 at 22:42, Nathan Bossart <nathandbossart@gmail.com>
wrote:

Committed.

Thanks a lot for the edits and backpatch.

Regards,
Ayush