From 75fc6d45934c1c226bb870a7d48778d1231b6d79 Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" Date: Thu, 3 Sep 2026 12:38:31 -0700 Subject: [PATCH v1] Improve the error hint for already-modified rows in FOR PORTION OF Normally when a trigger modifies the same row as the main statement, we fail with this message: ERROR: tuple to be updated was already modified by an operation triggered by the current command HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows. But if the trigger came from inserting a temporal leftover, it may have been an AFTER trigger instead. We can't tell which one it was at that point, so this commit keeps the hint and adds a detail naming the other possibility. The same detail is added to the matching check in GetTupleForTrigger. Reported-by: Noah Misch Author: Paul A. Jungwirth Backpatch-through: 19 --- src/backend/commands/trigger.c | 20 +++++ src/backend/executor/nodeModifyTable.c | 4 + src/include/commands/trigger.h | 2 + src/test/regress/expected/for_portion_of.out | 82 ++++++++++++++++++++ src/test/regress/sql/for_portion_of.sql | 72 +++++++++++++++++ 5 files changed, 180 insertions(+) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 3a085e8379c..ac81c3aea0d 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -3353,6 +3353,25 @@ ExecASTruncateTriggers(EState *estate, ResultRelInfo *relinfo) } +/* + * Explain an "already modified by an operation triggered by the current + * command" error when FOR PORTION OF is involved. + * + * The accompanying hint blames a BEFORE trigger, which is the only way an + * ordinary statement reaches these errors. But FOR PORTION OF gives a + * second way: temporal leftovers fire AFTER triggers before the main command + * completes. We can't tell which one it was here, so just add some extra + * detail under FOR PORTION OF. + */ +int +errdetail_temporal_leftovers(ResultRelInfo *relinfo) +{ + if (relinfo->ri_forPortionOf == NULL) + return 0; + + return errdetail("FOR PORTION OF inserts temporal leftovers while the statement is still running, and those inserts fire their own triggers."); +} + /* * Fetch tuple into "oldslot", dealing with locking and EPQ if necessary */ @@ -3414,6 +3433,7 @@ GetTupleForTrigger(EState *estate, ereport(ERROR, (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), errmsg("tuple to be updated was already modified by an operation triggered by the current command"), + errdetail_temporal_leftovers(relinfo), errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); /* treat it as deleted; do not process */ diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 5681505d31c..1785629dbe6 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1969,6 +1969,7 @@ ldelete: ereport(ERROR, (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), errmsg("tuple to be deleted was already modified by an operation triggered by the current command"), + errdetail_temporal_leftovers(resultRelInfo), errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); /* Else, already deleted by self; nothing to do */ @@ -2043,6 +2044,7 @@ ldelete: ereport(ERROR, (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), errmsg("tuple to be deleted was already modified by an operation triggered by the current command"), + errdetail_temporal_leftovers(resultRelInfo), errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); return NULL; @@ -2883,6 +2885,7 @@ redo_act: ereport(ERROR, (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), errmsg("tuple to be updated was already modified by an operation triggered by the current command"), + errdetail_temporal_leftovers(resultRelInfo), errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); /* Else, already updated by self; nothing to do */ @@ -2973,6 +2976,7 @@ redo_act: ereport(ERROR, (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION), errmsg("tuple to be updated was already modified by an operation triggered by the current command"), + errdetail_temporal_leftovers(resultRelInfo), errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); return NULL; diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index fecdb785f35..d54b82eab8e 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -258,6 +258,7 @@ extern void ExecBSTruncateTriggers(EState *estate, ResultRelInfo *relinfo); extern void ExecASTruncateTriggers(EState *estate, ResultRelInfo *relinfo); +extern int errdetail_temporal_leftovers(ResultRelInfo *relinfo); extern void AfterTriggerBeginXact(void); extern void AfterTriggerBeginQuery(void); @@ -277,6 +278,7 @@ extern bool RI_FKey_pk_upd_check_required(Trigger *trigger, Relation pk_rel, TupleTableSlot *oldslot, TupleTableSlot *newslot); extern bool RI_FKey_fk_upd_check_required(Trigger *trigger, Relation fk_rel, TupleTableSlot *oldslot, TupleTableSlot *newslot); + extern bool RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel); extern void RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, diff --git a/src/test/regress/expected/for_portion_of.out b/src/test/regress/expected/for_portion_of.out index 64789d1777b..07b91e454a6 100644 --- a/src/test/regress/expected/for_portion_of.out +++ b/src/test/regress/expected/for_portion_of.out @@ -2793,4 +2793,86 @@ SELECT * FROM fpo_rls ORDER BY valid_at; DROP TABLE fpo_rls; DROP ROLE regress_fpo_rls; +-- +-- Diagnostics when a trigger on the leftover INSERTs changes rows the +-- statement has not reached yet. +-- +-- Leftovers are inserted as the scan proceeds and fire their own triggers, so +-- an AFTER row trigger can trip the "already modified by an operation +-- triggered by the current command" check. The stock hint only mentions +-- BEFORE triggers, so we add a detail naming the other possibility. +-- +CREATE TABLE fpo_selfmod ( + id int, + valid_at daterange, + name text, + touched int NOT NULL DEFAULT 0 +); +INSERT INTO fpo_selfmod (id, valid_at, name) VALUES + (1, daterange('2000-01-01', '2010-01-01'), 'one'), + (2, daterange('2000-01-01', '2010-01-01'), 'two'); +CREATE FUNCTION fpo_touch_siblings() RETURNS TRIGGER LANGUAGE plpgsql AS +$$ +BEGIN + IF pg_trigger_depth() > 1 THEN + RETURN NULL; + END IF; + UPDATE fpo_selfmod SET touched = touched + 1 WHERE id <> NEW.id; + RETURN NULL; +END; +$$; +CREATE TRIGGER fpo_selfmod_ai AFTER INSERT ON fpo_selfmod + FOR EACH ROW EXECUTE PROCEDURE fpo_touch_siblings(); +UPDATE fpo_selfmod FOR PORTION OF valid_at FROM '2002-01-01' TO '2003-01-01' + SET name = name || '!'; +ERROR: tuple to be updated was already modified by an operation triggered by the current command +DETAIL: FOR PORTION OF inserts temporal leftovers while the statement is still running, and those inserts fire their own triggers. +HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows. +DELETE FROM fpo_selfmod FOR PORTION OF valid_at FROM '2002-01-01' TO '2003-01-01'; +ERROR: tuple to be deleted was already modified by an operation triggered by the current command +DETAIL: FOR PORTION OF inserts temporal leftovers while the statement is still running, and those inserts fire their own triggers. +HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows. +-- The same detail appears for the check in GetTupleForTrigger(), which a +-- BEFORE trigger reaches. There the stock hint is the right advice, so it is +-- still what we suggest. +CREATE FUNCTION fpo_touch_siblings_bu() RETURNS TRIGGER LANGUAGE plpgsql AS +$$ +BEGIN + IF pg_trigger_depth() = 1 THEN + UPDATE fpo_selfmod SET touched = touched + 1 WHERE id <> NEW.id; + END IF; + RETURN NEW; +END; +$$; +CREATE TRIGGER fpo_selfmod_bu BEFORE UPDATE ON fpo_selfmod + FOR EACH ROW EXECUTE PROCEDURE fpo_touch_siblings_bu(); +UPDATE fpo_selfmod FOR PORTION OF valid_at FROM '2002-01-01' TO '2003-01-01' + SET name = name || '!'; +ERROR: tuple to be updated was already modified by an operation triggered by the current command +DETAIL: FOR PORTION OF inserts temporal leftovers while the statement is still running, and those inserts fire their own triggers. +HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows. +DROP TRIGGER fpo_selfmod_bu ON fpo_selfmod; +DROP FUNCTION fpo_touch_siblings_bu(); +-- Deferring the trigger works. +DROP TRIGGER fpo_selfmod_ai ON fpo_selfmod; +CREATE CONSTRAINT TRIGGER fpo_selfmod_ai AFTER INSERT ON fpo_selfmod + DEFERRABLE INITIALLY DEFERRED + FOR EACH ROW EXECUTE PROCEDURE fpo_touch_siblings(); +BEGIN; +UPDATE fpo_selfmod FOR PORTION OF valid_at FROM '2002-01-01' TO '2003-01-01' + SET name = name || '!'; +COMMIT; +SELECT * FROM fpo_selfmod ORDER BY id, valid_at; + id | valid_at | name | touched +----+-------------------------+------+--------- + 1 | [2000-01-01,2002-01-01) | one | 2 + 1 | [2002-01-01,2003-01-01) | one! | 2 + 1 | [2003-01-01,2010-01-01) | one | 2 + 2 | [2000-01-01,2002-01-01) | two | 2 + 2 | [2002-01-01,2003-01-01) | two! | 2 + 2 | [2003-01-01,2010-01-01) | two | 2 +(6 rows) + +DROP TABLE fpo_selfmod; +DROP FUNCTION fpo_touch_siblings(); RESET datestyle; diff --git a/src/test/regress/sql/for_portion_of.sql b/src/test/regress/sql/for_portion_of.sql index b61fe10478e..e2cba6f8012 100644 --- a/src/test/regress/sql/for_portion_of.sql +++ b/src/test/regress/sql/for_portion_of.sql @@ -1849,4 +1849,76 @@ SELECT * FROM fpo_rls ORDER BY valid_at; DROP TABLE fpo_rls; DROP ROLE regress_fpo_rls; +-- +-- Diagnostics when a trigger on the leftover INSERTs changes rows the +-- statement has not reached yet. +-- +-- Leftovers are inserted as the scan proceeds and fire their own triggers, so +-- an AFTER row trigger can trip the "already modified by an operation +-- triggered by the current command" check. The stock hint only mentions +-- BEFORE triggers, so we add a detail naming the other possibility. +-- + +CREATE TABLE fpo_selfmod ( + id int, + valid_at daterange, + name text, + touched int NOT NULL DEFAULT 0 +); +INSERT INTO fpo_selfmod (id, valid_at, name) VALUES + (1, daterange('2000-01-01', '2010-01-01'), 'one'), + (2, daterange('2000-01-01', '2010-01-01'), 'two'); + +CREATE FUNCTION fpo_touch_siblings() RETURNS TRIGGER LANGUAGE plpgsql AS +$$ +BEGIN + IF pg_trigger_depth() > 1 THEN + RETURN NULL; + END IF; + UPDATE fpo_selfmod SET touched = touched + 1 WHERE id <> NEW.id; + RETURN NULL; +END; +$$; + +CREATE TRIGGER fpo_selfmod_ai AFTER INSERT ON fpo_selfmod + FOR EACH ROW EXECUTE PROCEDURE fpo_touch_siblings(); + +UPDATE fpo_selfmod FOR PORTION OF valid_at FROM '2002-01-01' TO '2003-01-01' + SET name = name || '!'; +DELETE FROM fpo_selfmod FOR PORTION OF valid_at FROM '2002-01-01' TO '2003-01-01'; + +-- The same detail appears for the check in GetTupleForTrigger(), which a +-- BEFORE trigger reaches. There the stock hint is the right advice, so it is +-- still what we suggest. +CREATE FUNCTION fpo_touch_siblings_bu() RETURNS TRIGGER LANGUAGE plpgsql AS +$$ +BEGIN + IF pg_trigger_depth() = 1 THEN + UPDATE fpo_selfmod SET touched = touched + 1 WHERE id <> NEW.id; + END IF; + RETURN NEW; +END; +$$; +CREATE TRIGGER fpo_selfmod_bu BEFORE UPDATE ON fpo_selfmod + FOR EACH ROW EXECUTE PROCEDURE fpo_touch_siblings_bu(); +UPDATE fpo_selfmod FOR PORTION OF valid_at FROM '2002-01-01' TO '2003-01-01' + SET name = name || '!'; +DROP TRIGGER fpo_selfmod_bu ON fpo_selfmod; +DROP FUNCTION fpo_touch_siblings_bu(); + +-- Deferring the trigger works. +DROP TRIGGER fpo_selfmod_ai ON fpo_selfmod; +CREATE CONSTRAINT TRIGGER fpo_selfmod_ai AFTER INSERT ON fpo_selfmod + DEFERRABLE INITIALLY DEFERRED + FOR EACH ROW EXECUTE PROCEDURE fpo_touch_siblings(); + +BEGIN; +UPDATE fpo_selfmod FOR PORTION OF valid_at FROM '2002-01-01' TO '2003-01-01' + SET name = name || '!'; +COMMIT; +SELECT * FROM fpo_selfmod ORDER BY id, valid_at; + +DROP TABLE fpo_selfmod; +DROP FUNCTION fpo_touch_siblings(); + RESET datestyle; -- 2.47.3