Hi!
Thank you for the review!
3. Consider updating documentation for doc/src/contrib-spi.sgml, or any
file as appropriate, to reflect the changes.
The changes have now been added to doc/src/contrib-spi.sgml. I also
added a consideration note about interactions with BEFORE triggers.
4. Are there any cases where check_primary_key() and
check_foreign_key() should be called using a BEFORE trigger? Will this
change break backwards compatibility? Consider adding a test with a
BEFORE trigger to ensure the error "must be fired by AFTER trigger" is
raised.
The usage within BEFORE triggers appears to be entirely incorrect. The
functions check_primary_key() and check_foreign_key() are intended for
use in creating constraint triggers, which according to the
documentation must be AFTER ROW triggers. Therefore, any cases using
BEFORE triggers are invalid.
I have updated the test so that it now raises the error "must be fired
by AFTER trigger."
Can you also help me with the patch status? What status should I move
the patch to?From f94391f6812d6a9bbdd2c43f5b24373c08cb08bb Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bon...@postgrespro.ru>
Date: Wed, 26 Mar 2025 16:39:01 +0700
Subject: [PATCH v5] Triggers test fix with the invalid cache in refint.c
---
contrib/spi/refint.c | 25 +++++++----
doc/src/sgml/contrib-spi.sgml | 12 +++++-
src/test/regress/expected/triggers.out | 57 +++++++++++++++++---------
src/test/regress/sql/triggers.sql | 30 +++++++++++---
4 files changed, 90 insertions(+), 34 deletions(-)
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index e1aef7cd2a3..fd4ba1c0698 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -81,6 +81,10 @@ check_primary_key(PG_FUNCTION_ARGS)
/* internal error */
elog(ERROR, "check_primary_key: must be fired for row");
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ /* internal error */
+ elog(ERROR, "check_primary_key: must be fired by AFTER trigger");
+
/* If INSERTion then must check Tuple to being inserted */
if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
tuple = trigdata->tg_trigtuple;
@@ -264,7 +268,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
#ifdef DEBUG_QUERY
elog(DEBUG4, "check_foreign_key: Enter Function");
#endif
-
/*
* Some checks first...
*/
@@ -284,6 +287,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
/* internal error */
elog(ERROR, "check_foreign_key: cannot process INSERT events");
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ /* internal error */
+ elog(ERROR, "check_foreign_key: must be fired by AFTER trigger");
+
/* Have to check tg_trigtuple - tuple being deleted */
trigtuple = trigdata->tg_trigtuple;
@@ -335,10 +342,10 @@ check_foreign_key(PG_FUNCTION_ARGS)
kvals = (Datum *) palloc(nkeys * sizeof(Datum));
/*
- * Construct ident string as TriggerName $ TriggeredRelationId and try to
- * find prepared execution plan(s).
+ * Construct ident string as TriggerName $ TriggeredRelationId $ OperationType
+ * and try to find prepared execution plan(s).
*/
- snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
+ snprintf(ident, sizeof(ident), "%s$%u$%c", trigger->tgname, rel->rd_id, is_update ? 'U' : 'D');
plan = find_plan(ident, &FPlans, &nFPlans);
/* if there is no plan(s) then allocate argtypes for preparation */
@@ -570,7 +577,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
relname = args[0];
- snprintf(ident, sizeof(ident), "%s$%u", trigger->tgname, rel->rd_id);
plan = find_plan(ident, &FPlans, &nFPlans);
ret = SPI_execp(plan->splan[r], kvals, NULL, tcount);
/* we have no NULLs - so we pass ^^^^ here */
@@ -592,10 +598,15 @@ check_foreign_key(PG_FUNCTION_ARGS)
}
else
{
+ const char* operation;
+
+ if (action == 'c')
+ operation = is_update ? "updated" : "deleted";
+ else
+ operation = "set to null";
#ifdef REFINT_VERBOSE
elog(NOTICE, "%s: " UINT64_FORMAT " tuple(s) of %s are %s",
- trigger->tgname, SPI_processed, relname,
- (action == 'c') ? "deleted" : "set to null");
+ trigger->tgname, SPI_processed, relname, operation);
#endif
}
args += nkeys + 1; /* to the next relation */
diff --git a/doc/src/sgml/contrib-spi.sgml b/doc/src/sgml/contrib-spi.sgml
index e7cae4e38dc..4fcc767a978 100644
--- a/doc/src/sgml/contrib-spi.sgml
+++ b/doc/src/sgml/contrib-spi.sgml
@@ -36,7 +36,7 @@
<para>
<function>check_primary_key()</function> checks the referencing table.
- To use, create a <literal>BEFORE INSERT OR UPDATE</literal> trigger using this
+ To use, create a <literal>AFTER INSERT OR UPDATE</literal> trigger using this
function on a table referencing another table. Specify as the trigger
arguments: the referencing table's column name(s) which form the foreign
key, the referenced table name, and the column names in the referenced table
@@ -46,7 +46,7 @@
<para>
<function>check_foreign_key()</function> checks the referenced table.
- To use, create a <literal>BEFORE DELETE OR UPDATE</literal> trigger using this
+ To use, create a <literal>AFTER DELETE OR UPDATE</literal> trigger using this
function on a table referenced by other table(s). Specify as the trigger
arguments: the number of referencing tables for which the function has to
perform checking, the action if a referencing key is found
@@ -63,6 +63,14 @@
<para>
There are examples in <filename>refint.example</filename>.
</para>
+
+ <para>
+ Note that if these triggers are executed from another <literal>BEFORE</literal>
+ trigger, they can fail unexpectedly. For example, if a user inserts row1 and then
+ the <literal>BEFORE</literal> trigger inserts row2 and calls a trigger with the
+ <function>check_foreign_key()</function>, the <function>check_foreign_key()</function>
+ function will not see row1 and will fail.
+ </para>
</sect2>
<sect2 id="contrib-spi-autoinc">
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 247c67c32ae..3b8bc558140 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -46,12 +46,12 @@ create unique index pkeys_i on pkeys (pkey1, pkey2);
-- (fkey3) --> fkeys2 (pkey23)
--
create trigger check_fkeys_pkey_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function
check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2');
create trigger check_fkeys_pkey2_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23');
--
@@ -59,7 +59,7 @@ create trigger check_fkeys_pkey2_exist
-- (fkey21, fkey22) --> pkeys (pkey1, pkey2)
--
create trigger check_fkeys2_pkey_exist
- before insert or update on fkeys2
+ after insert or update on fkeys2
for each row
execute procedure
check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
@@ -74,7 +74,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL;
-- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22)
--
create trigger check_pkeys_fkey_cascade
- before delete or update on pkeys
+ after delete or update on pkeys
for each row
execute procedure
check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
@@ -85,9 +85,27 @@ create trigger check_pkeys_fkey_cascade
-- fkeys (fkey3)
--
create trigger check_fkeys2_fkey_restrict
- before delete or update on fkeys2
+ after delete or update on fkeys2
for each row
execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');
+-- BEFORE triggers must raise an error
+create trigger check_fkeys2_pkey_exist_before
+ before insert or update on fkeys2
+ for each row
+ execute procedure
+ check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
+create trigger check_pkeys_fkey_cascade_before
+ before delete or update on pkeys
+ for each row
+ execute procedure
+ check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
+ 'fkeys', 'fkey1', 'fkey2', 'fkeys2', 'fkey21', 'fkey22');
+update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
+ERROR: check_foreign_key: must be fired by AFTER trigger
+insert into fkeys2 values (10, '1', 1);
+ERROR: check_primary_key: must be fired by AFTER trigger
+drop trigger check_fkeys2_pkey_exist_before on fkeys2;
+drop trigger check_pkeys_fkey_cascade_before on pkeys;
insert into fkeys2 values (10, '1', 1);
insert into fkeys2 values (30, '3', 2);
insert into fkeys2 values (40, '4', 5);
@@ -116,12 +134,11 @@ delete from pkeys where pkey1 = 40 and pkey2 = '4';
NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted
update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 50 and pkey2 = '5';
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
-ERROR: "check_fkeys2_fkey_restrict": tuple is referenced in "fkeys"
-CONTEXT: SQL statement "delete from fkeys2 where fkey21 = $1 and fkey22 = $2 "
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are updated
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are updated
update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are deleted
-NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are deleted
+ERROR: duplicate key value violates unique constraint "pkeys_i"
+DETAIL: Key (pkey1, pkey2)=(7, 70) already exists.
SELECT trigger_name, event_manipulation, event_object_schema, event_object_table,
action_order, action_condition, action_orientation, action_timing,
action_reference_old_table, action_reference_new_table
@@ -130,16 +147,16 @@ SELECT trigger_name, event_manipulation, event_object_schema, event_object_table
ORDER BY trigger_name COLLATE "C", 2;
trigger_name | event_manipulation | event_object_schema | event_object_table | action_order | action_condition | action_orientation | action_timing | action_reference_old_table | action_reference_new_table
----------------------------+--------------------+---------------------+--------------------+--------------+------------------+--------------------+---------------+----------------------------+----------------------------
- check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | BEFORE | |
- check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | BEFORE | |
- check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | BEFORE | |
- check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | BEFORE | |
- check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | BEFORE | |
- check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | BEFORE | |
- check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | BEFORE | |
- check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | BEFORE | |
+ check_fkeys2_fkey_restrict | DELETE | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_fkey_restrict | UPDATE | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_pkey_exist | INSERT | public | fkeys2 | 1 | | ROW | AFTER | |
+ check_fkeys2_pkey_exist | UPDATE | public | fkeys2 | 2 | | ROW | AFTER | |
+ check_fkeys_pkey2_exist | INSERT | public | fkeys | 1 | | ROW | AFTER | |
+ check_fkeys_pkey2_exist | UPDATE | public | fkeys | 1 | | ROW | AFTER | |
+ check_fkeys_pkey_exist | INSERT | public | fkeys | 2 | | ROW | AFTER | |
+ check_fkeys_pkey_exist | UPDATE | public | fkeys | 2 | | ROW | AFTER | |
+ check_pkeys_fkey_cascade | DELETE | public | pkeys | 1 | | ROW | AFTER | |
+ check_pkeys_fkey_cascade | UPDATE | public | pkeys | 1 | | ROW | AFTER | |
(10 rows)
DROP TABLE pkeys;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 659972f1135..7c9f6ec3b7e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -57,13 +57,13 @@ create unique index pkeys_i on pkeys (pkey1, pkey2);
-- (fkey3) --> fkeys2 (pkey23)
--
create trigger check_fkeys_pkey_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function
check_primary_key ('fkey1', 'fkey2', 'pkeys', 'pkey1', 'pkey2');
create trigger check_fkeys_pkey2_exist
- before insert or update on fkeys
+ after insert or update on fkeys
for each row
execute function check_primary_key ('fkey3', 'fkeys2', 'pkey23');
@@ -72,7 +72,7 @@ create trigger check_fkeys_pkey2_exist
-- (fkey21, fkey22) --> pkeys (pkey1, pkey2)
--
create trigger check_fkeys2_pkey_exist
- before insert or update on fkeys2
+ after insert or update on fkeys2
for each row
execute procedure
check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
@@ -88,7 +88,7 @@ COMMENT ON TRIGGER check_fkeys2_pkey_exist ON fkeys2 IS NULL;
-- fkeys (fkey1, fkey2) and fkeys2 (fkey21, fkey22)
--
create trigger check_pkeys_fkey_cascade
- before delete or update on pkeys
+ after delete or update on pkeys
for each row
execute procedure
check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
@@ -100,10 +100,30 @@ create trigger check_pkeys_fkey_cascade
-- fkeys (fkey3)
--
create trigger check_fkeys2_fkey_restrict
- before delete or update on fkeys2
+ after delete or update on fkeys2
for each row
execute procedure check_foreign_key (1, 'restrict', 'pkey23', 'fkeys', 'fkey3');
+-- BEFORE triggers must raise an error
+create trigger check_fkeys2_pkey_exist_before
+ before insert or update on fkeys2
+ for each row
+ execute procedure
+ check_primary_key ('fkey21', 'fkey22', 'pkeys', 'pkey1', 'pkey2');
+
+create trigger check_pkeys_fkey_cascade_before
+ before delete or update on pkeys
+ for each row
+ execute procedure
+ check_foreign_key (2, 'cascade', 'pkey1', 'pkey2',
+ 'fkeys', 'fkey1', 'fkey2', 'fkeys2', 'fkey21', 'fkey22');
+
+update pkeys set pkey1 = 7, pkey2 = '70' where pkey1 = 10 and pkey2 = '1';
+insert into fkeys2 values (10, '1', 1);
+
+drop trigger check_fkeys2_pkey_exist_before on fkeys2;
+drop trigger check_pkeys_fkey_cascade_before on pkeys;
+
insert into fkeys2 values (10, '1', 1);
insert into fkeys2 values (30, '3', 2);
insert into fkeys2 values (40, '4', 5);
--
2.34.1
