Dmitrii Bondar писал(а) 2025-01-29 16:53:
Hi, Hackers!
I was testing a connection pooler with `make installcheck` and noticed
that `check_foreign_key()` from the `refint` library reuses the same
cached plan for cascade `UPDATE`/`DELETE` operations. As a result, a
cascade `DELETE` is applied after an `UPDATE` command on the primary
key table (which should not happen after the commit
https://github.com/postgres/postgres/commit/d489fdfc7f4ccf0010fe0397e7272bdfc257e8f2).
I have attached a file (test.sql) to reproduce an issue and the
solution.
Regards,
Dmitrii Bondar.
Found a mistake. Now it should work even if the SPI call fails (v2
attachment). However, the whole solution looks awkward because if
`check_primary_key` is triggered by a function other than
check_foreign_key, we still encounter invalid behavior. The root of the
problem is the inability to see the row that triggered the initial
`check_foreign_key`.
I am also considering another solution (v3 attachment): instead of using
static variables, restrict the use of the `check_primary_key` and
`check_foreign` functions in BEFORE triggers so that the
`check_primary_key` trigger can find the new row. This still doesn't
solve the problem (a user could create their own BEFORE trigger that
make `UPDATE` and trigger `check_primary_key`), but it adds less new
code, at least.
Regards,
Dmitrii Bondar
From 054b2945dc7d47328fb972c725a3a89a0af300a9 Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bon...@postgrespro.ru>
Date: Mon, 3 Feb 2025 15:45:21 +0700
Subject: [PATCH v2] Triggers test fix
---
contrib/spi/refint.c | 29 +++++++++++++++++++-------
src/test/regress/expected/triggers.out | 11 +++++-----
2 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index e1aef7cd2a..7344f6c742 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -29,6 +29,9 @@ static int nFPlans = 0;
static EPlan *PPlans = NULL;
static int nPPlans = 0;
+static bool update_in_progress = false;
+static TransactionId update_tx = InvalidTransactionId;
+
static EPlan *find_plan(char *ident, EPlan **eplan, int *nplans);
/*
@@ -98,6 +101,10 @@ check_primary_key(PG_FUNCTION_ARGS)
nargs = trigger->tgnargs;
args = trigger->tgargs;
+ /* Do not check if It's a cascade update from check_foreign_key */
+ if (update_in_progress && update_tx == GetCurrentTransactionId())
+ return PointerGetDatum(tuple);
+
if (nargs % 2 != 1) /* odd number of arguments! */
/* internal error */
elog(ERROR, "check_primary_key: odd number of arguments should be specified");
@@ -264,7 +271,6 @@ check_foreign_key(PG_FUNCTION_ARGS)
#ifdef DEBUG_QUERY
elog(DEBUG4, "check_foreign_key: Enter Function");
#endif
-
/*
* Some checks first...
*/
@@ -335,10 +341,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 */
@@ -560,6 +566,9 @@ check_foreign_key(PG_FUNCTION_ARGS)
/*
* Ok, execute prepared plan(s).
*/
+ if (is_update)
+ update_tx = GetCurrentTransactionId();
+
for (r = 0; r < nrefs; r++)
{
/*
@@ -570,10 +579,11 @@ 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);
+ update_in_progress = (is_update == 1);
ret = SPI_execp(plan->splan[r], kvals, NULL, tcount);
/* we have no NULLs - so we pass ^^^^ here */
+ update_in_progress = false;
if (ret < 0)
ereport(ERROR,
@@ -592,10 +602,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/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index e38304bee5..cc02b81bb2 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -116,12 +116,13 @@ 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
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys are updated
+NOTICE: check_pkeys_fkey_cascade: 1 tuple(s) of fkeys2 are updated
+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
--
2.34.1
From e1573beaac21fd790f05ef3cd6e7e51d783ec00d Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bon...@postgrespro.ru>
Date: Tue, 4 Feb 2025 10:57:12 +0700
Subject: [PATCH v3] Triggers test fix
---
contrib/spi/refint.c | 25 ++++++++++++-----
src/test/regress/expected/triggers.out | 39 +++++++++++++-------------
src/test/regress/sql/triggers.sql | 10 +++----
3 files changed, 42 insertions(+), 32 deletions(-)
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index e1aef7cd2a..fd4ba1c069 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/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index e38304bee5..9dbb3bd3cd 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,7 +85,7 @@ 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');
insert into fkeys2 values (10, '1', 1);
@@ -116,12 +116,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 +129,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 b79fecb8fe..f2effa91f9 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,7 +100,7 @@ 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');
--
2.34.1
