[fixed] Trigger test
Started by Dmitrii Bondar12 months ago11 messages
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.
Attachments:
v1-0001-Triggers-test-fix.patchtext/x-diff; name=v1-0001-Triggers-test-fix.patchDownload
From 57e13d3dde3b8d071347bb8f46298bd32ab2bbed Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bondar@postgrespro.ru>
Date: Wed, 29 Jan 2025 16:31:56 +0700
Subject: [PATCH v1] Triggers test fix
---
contrib/spi/refint.c | 23 +++++++++++++++++------
src/test/regress/expected/triggers.out | 11 ++++++-----
2 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index e1aef7cd2a..45a80bfacb 100644
--- a/contrib/spi/refint.c
+++ b/contrib/spi/refint.c
@@ -28,6 +28,7 @@ static EPlan *FPlans = NULL;
static int nFPlans = 0;
static EPlan *PPlans = NULL;
static int nPPlans = 0;
+static bool update_in_progress = false;
static EPlan *find_plan(char *ident, EPlan **eplan, int *nplans);
@@ -98,6 +99,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)
+ 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");
@@ -335,10 +340,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,10 +575,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 +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..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
Re: [fixed] Trigger test
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
Attachments:
v2-0001-Triggers-test-fix.patchtext/x-diff; name=v2-0001-Triggers-test-fix.patchDownload
From 054b2945dc7d47328fb972c725a3a89a0af300a9 Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bondar@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
v3-0001-Triggers-test-fix.patchtext/x-diff; name=v3-0001-Triggers-test-fix.patchDownload
From e1573beaac21fd790f05ef3cd6e7e51d783ec00d Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bondar@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
Re: [fixed] Trigger test
Just a rebase.
Attachments:
v4-0001-Triggers-test-fix.patchtext/x-diff; name=v4-0001-Triggers-test-fix.patchDownload
From 76c4ca0f63091551b3f579c2c74345438e3d62d7 Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bondar@postgrespro.ru>
Date: Wed, 19 Feb 2025 09:37:48 +0700
Subject: [PATCH v4] 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 247c67c32a..e6f585d974 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 659972f113..e5a491be7a 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
Re: [fixed] Trigger test
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
Hi Dmitrii,
Paul Jungwirth and I reviewed this patch, and here are our comments:
1. The patch applies and tests pass.
2. The patch fixes a bug in contrib/spi, which is not really a practical extension, but rather examples of how to use SPI. The contrib/spi directory actually has four extensions: refint, autoinc, insert_username, and moddatetime. The patch is for refint, which is a way you could implement foreign keys if it weren't built in to Postgres.
3. Consider updating documentation for doc/src/contrib-spi.sgml, or any file as appropriate, to reflect the changes.
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.
Thank you!
The new status of this patch is: Waiting on Author
Re: [fixed] Trigger test
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?
Attachments:
v5-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patchtext/x-diff; name=v5-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patchDownload
From f94391f6812d6a9bbdd2c43f5b24373c08cb08bb Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bondar@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
Re: [fixed] Trigger test
Hi Dmitrii,
Thanks for the quick update!
On 3/26/25 02:45, Dmitrii Bondar wrote:
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.
This looks good. I have a couple small grammar suggestions. This:
+ To use, create a <literal>AFTER INSERT OR UPDATE</literal> trigger using this
should be:
+ To use, create an <literal>AFTER INSERT OR UPDATE</literal> trigger using this
and this:
+ To use, create a <literal>AFTER DELETE OR UPDATE</literal> trigger using this
should be this:
+ To use, create an <literal>AFTER DELETE OR UPDATE</literal> trigger using this
Also re this part of the patch:
@@ -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 */We can put all the new lines inside the #ifdef, can't we?
Can you also help me with the patch status? What status should I move the patch to?
I think if you make those changes we should mark this as Ready for Committer.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Re: [fixed] Trigger test
Hi, Paul,
Thanks for the suggestions.
This looks good. I have a couple small grammar suggestions. This:
I have replaced the incorrect articles with the correct ones.
We can put all the new lines inside the #ifdef, can't we?
You're right. I have done that.
Best regards,
Dmitrii
Attachments:
v6-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patchtext/x-patch; charset=UTF-8; name=v6-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patchDownload
From f566662d52fcc6c2febb54dfb6f3aa5e128bcea1 Mon Sep 17 00:00:00 2001
From: Bondar Dmitrii <d.bondar@postgrespro.ru>
Date: Thu, 27 Mar 2025 14:58:49 +0700
Subject: [PATCH v6] Triggers test fix with the invalid cache in refint.c
---
contrib/spi/refint.c | 26 ++++++++----
doc/src/sgml/contrib-spi.sgml | 12 +++++-
src/test/regress/expected/triggers.out | 57 +++++++++++++++++---------
src/test/regress/sql/triggers.sql | 30 +++++++++++---
4 files changed, 91 insertions(+), 34 deletions(-)
diff --git a/contrib/spi/refint.c b/contrib/spi/refint.c
index e1aef7cd2a3..b1e697eb935 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 */
@@ -593,9 +599,15 @@ check_foreign_key(PG_FUNCTION_ARGS)
else
{
#ifdef REFINT_VERBOSE
+ const char* operation;
+
+ if (action == 'c')
+ operation = is_update ? "updated" : "deleted";
+ else
+ operation = "set to null";
+
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..3a838199484 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 an <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 an <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
Re: [fixed] Trigger test
Dmitrii Bondar <d.bondar@postgrespro.ru> writes:
[ v6-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patch ]
I spent a little bit of time looking over this patch. My first
instinct was "we can't really change the recommended method of
installing these triggers" --- but that would only matter if we
thought there were actual production users of these triggers,
which surely there are not (anymore). The only reason we're
carrying refint.c at all is as an example of C-coded triggers.
So changing the example seems fine, and you're right that this
sort of change is far better done from an AFTER trigger.
However ... as an example refint.c is pretty darn awful :-(.
I'd never looked hard at it before, and now that I have,
I'm rather horrified, particularly by the shoddy quoting practices.
None of the identifiers inserted into constructed queries receive
quote_identifier() protection, and the insertion of data values
around line 500 is beyond awful.
So while I think your v6 patch fixes the problem(s) it set out
to fix, it still feels a lot like putting lipstick on a pig.
I wonder if we'd be better off to nuke refint.c altogether.
If we don't, I feel like we're morally obliged to spend more
effort trying to make it less of an example of bad practices.
Some of the things I think need to be cleaned up:
* It's ridiculous that the update-cascade case is inserting
data values textually at all. Even if it were quoting them
properly, that's expensive and risks round-trip-conversion
problems. That should be handled as an additional Param value
passed into the statement.
* Worse yet, that code doesn't work if used more than once,
because the first value that needs to be updated gets baked
into the plan that will be re-used later. So the Param
approach is really essential even aside from quoting concerns.
* String comparisons to detect value equality (around line 400)
are not terribly cool either. Proper code would be looking up
the default equality operator for the datatypes and applying that.
* Some thought needs to be given to table and column names that
require quoting. I guess in a way there's an advantage to not
quoting the table names that come from trigger arguments: it
lets the user get around the module's failure to think about
schema-qualified names, by writing 'foo.bar' rather than just
'bar'. But that's not documented. If we did quote everything
then we'd really have to go over to providing separate schema
and name arguments for each of the other tables. In any case,
not quoting the column names has nothing to recommend it.
* I'll slide gently past the question of whether this should
be expected to react on-the-fly to DDL changes in the tables.
SPI will do some of that under the hood, but it can't fix
cases where the query string would need to change (eg.
table or column renames).
So that's a long laundry list and we haven't even dug hard.
Is it worth it? If you feel like doing the legwork then
I'm willing to support the project, but I really wonder if
we shouldn't cut our losses and just remove the module.
(I hesitate now to look at the rest of contrib/spi/ :-()
regards, tom lane
Re: [fixed] Trigger test
On 04/04/2025 01:11, Tom Lane wrote:
So that's a long laundry list and we haven't even dug hard.
Is it worth it? If you feel like doing the legwork then
I'm willing to support the project, but I really wonder if
we shouldn't cut our losses and just remove the module.(I hesitate now to look at the rest of contrib/spi/ :-()
You wrote a note that I decided to omit. As I mentioned, the patch does
not even fix the cascade update problem—there are still broken
cases—because it seems impossible to address it in a gentle way (the
code was patched 20 years ago; it's truly legacy).
I considered removing it entirely, but that seemed too drastic a
solution (and, at the very least, I don't have enough expertise to make
that decision). If everything looks acceptable, I would prefer to cut
the module. The |check_primary_key| and |check_foreign| functions are
clearly unused, are buggy, and no one has reported any obvious
problems—so refint.c can be safely removed. Autoinc.c also looks
problematic.
There are some question. When should we remove the module? Should we
mark it as deprecated for now and remove it later? Should we handle it
in another thread? Should we apply this patch in that case?
Best regards,
Dmitrii
Re: [fixed] Trigger test
Dmitrii Bondar <d.bondar@postgrespro.ru> writes:
On 04/04/2025 01:11, Tom Lane wrote:
So that's a long laundry list and we haven't even dug hard.
Is it worth it? If you feel like doing the legwork then
I'm willing to support the project, but I really wonder if
we shouldn't cut our losses and just remove the module.
You wrote a note that I decided to omit. As I mentioned, the patch does
not even fix the cascade update problem—there are still broken
cases—because it seems impossible to address it in a gentle way (the
code was patched 20 years ago; it's truly legacy).
I'm not terribly concerned about whether these triggers have perfect
foreign-key semantics, since no one (in their right mind) would use
them as foreign-key enforcement anyway. What they're good for
is as examples of writing checks and updates in C-coded triggers.
As such, questions like "are identifiers and data values quoted
appropriately" seem far more urgent than whether cascade update
works per spec. Even just using a StringInfo rather than a fixed-size
char[] variable to build the query in would be an improvement.
I considered removing it entirely, but that seemed too drastic a
solution (and, at the very least, I don't have enough expertise to make
that decision).
I'm not that thrilled with giving up on refint.c either. But in its
current state, it's a pretty lousy example. Are we willing to put
enough effort into making it a more useful code example?
regards, tom lane
Re: [fixed] Trigger test
I wrote:
Dmitrii Bondar <d.bondar@postgrespro.ru> writes:
I considered removing it entirely, but that seemed too drastic a
solution (and, at the very least, I don't have enough expertise to make
that decision).
I'm not that thrilled with giving up on refint.c either. But in its
current state, it's a pretty lousy example. Are we willing to put
enough effort into making it a more useful code example?
I concluded that we might as well commit what we've got, since the
window for v18 is closing fast and there's not time to consider
anything more invasive. Hence, pushed. I hope you will consider
spending some effort on fixing the other problems identified in
this thread.
regards, tom lane