public inbox for [email protected]
help / color / mirror / Atom feedFrom: Paul A Jungwirth <[email protected]>
To: SATYANARAYANA NARLAPURAM <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Chao Li <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: jian he <[email protected]>
Subject: Re: SQL:2011 Application Time Update & Delete
Date: Sat, 18 Apr 2026 16:18:13 -0700
Message-ID: <CA+renyU5=mihx6O8+ERBmarZcuV1QiWB3X5bZhdptWoRM9G-Aw@mail.gmail.com> (raw)
In-Reply-To: <CA+renyVkfsrNNnYqLpf_g3mDV31KLFXAw-RRVmyLb7TcBLUO7A@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CA+renyUazgR-hB_6RY60n23L0y-n_h9G1AappZmPENO0k5pL1g@mail.gmail.com>
<[email protected]>
<CA+renyVXg5pV84wQnGQuK8-=qoKw3BiBgQzesxM_LkcxxWmYjA@mail.gmail.com>
<[email protected]>
<CA+renyWKOj5=rMmQmJcbybu-Vdomxdp=eJ93kp76AgmQKYdfiQ@mail.gmail.com>
<[email protected]>
<CA+renyUhuXB2nTVCMREXew9E4DZOnFxQNjME5bcw91+k72Bosg@mail.gmail.com>
<CA+renyWUCSyTMn3s03kviEN-oaVrJP-QkDQCLNfaY=MHV5QEiQ@mail.gmail.com>
<CA+renyV4tWU2d=n9_v=XNPHbZfNqqLokzd-Xt78M-zLd+46ubA@mail.gmail.com>
<[email protected]>
<CA+renyUSgqXpjj+vV7w+wirPB49VQFrmPjVT_s04JmZSOPNNsQ@mail.gmail.com>
<[email protected]>
<CA+renyX-eV+2hFUaZg3BSREqLE7dh+LoWm7ZqhFAiGsirjjtRQ@mail.gmail.com>
<[email protected]>
<CAHg+QDckLFqthQyox2NDetYRs9sRrjmAiSA-gYRowyg8w_4vgw@mail.gmail.com>
<CA+renyV=ryhYnxgwwWWPEk0GfHpSS_xWZVx9wmvrWozpEmnOxg@mail.gmail.com>
<CA+renyVkfsrNNnYqLpf_g3mDV31KLFXAw-RRVmyLb7TcBLUO7A@mail.gmail.com>
On Wed, Apr 15, 2026 at 10:30 AM Paul A Jungwirth
<[email protected]> wrote:
>
> On Tue, Apr 14, 2026 at 10:34 PM Paul A Jungwirth
> <[email protected]> wrote:
> >
> > > A BEFORE UPDATE trigger that modifies the range column creates overlapping rows. The trigger widening the range doesn't affect leftover computation, which uses the original FPO bounds. Result: updated row overlaps both leftovers.
> >
> > I'm working on a fix for this. It's not quite ready, but I can finish
> > it in the morning. . . .
>
> Actually I think the proper behavior here is to raise an error. We
> forbid setting the application-time column when using FOR PORTION OF
> (per the standard), so why should we allow a BEFORE trigger to set it?
> I think it has the same inconsistency problems. We could support it,
> but then why not support both?
>
> Assuming we want to raise an error, I think the best way is to check
> the tuple in ExecForPortionOfLeftovers to see if a trigger has
> modified it, and in that case raise an error. What do you think?
Here is a patch that forbids changing the valid_at column in a BEFORE
trigger. It works by capturing the value before triggers run, then
checking afterwards if it is still the same (using the default btree
equality operator; probably a simple binary comparison is good
enough).
This copy+check only happens if the table has BEFORE UPDATE row
triggers, so there is no cost in most cases.
I'm raising ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION, which is what we
use when (basically) a trigger & UPDATE both change a row in a way
that leaves the user intent unclear. I think that's a very close fit
here, but you could argue we should use the same errcode as SETing
valid_at. That is ERRCODE_SYNTAX_ERROR. That strikes me as a
questionable choice, actually. Personally I think using different
errcodes is correct though.
In ExecForPortionOfSaveRange there is a lot of code duplication
copying the structure for child partitions, but I think we could cut
that by first adding jian he's helper function (ExecInitForPortionOf)
from another bugfix patch [1].
[1] https://www.postgresql.org/message-id/CA%2BrenyWD%2BXXifwswE74vhjooqbiVKu4qVhLvpMcUQBzrjVjT7A%40mail...
Yours,
--
Paul ~{:-)
[email protected]
Attachments:
[text/x-patch] v1-0001-Forbid-BEFORE-UPDATE-triggers-changing-the-FOR-PO.patch (12.1K, 2-v1-0001-Forbid-BEFORE-UPDATE-triggers-changing-the-FOR-PO.patch)
download | inline diff:
From d1f93cbc5018c41b0948ece5eade08583afe6ae3 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <[email protected]>
Date: Thu, 16 Apr 2026 13:00:59 -0700
Subject: [PATCH v1] Forbid BEFORE UPDATE triggers changing the FOR PORTION OF
column
Just as we forbid UPDATE t FOR PORTION OF valid_at ... SET valid_at, we
should forbid setting the application-time column with a BEFORE trigger.
We record the value before triggers fire, and then we compare
afterwards to make sure it hasn't been altered. If so we raise an error.
---
src/backend/executor/nodeModifyTable.c | 159 +++++++++++++++++--
src/include/nodes/execnodes.h | 5 +
src/test/regress/expected/for_portion_of.out | 20 +++
src/test/regress/sql/for_portion_of.sql | 24 +++
4 files changed, 196 insertions(+), 12 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index ef2a6bc6e9d..c82dea2eff1 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -171,7 +171,11 @@ static bool ExecOnConflictSelect(ModifyTableContext *context,
static void ExecForPortionOfLeftovers(ModifyTableContext *context,
EState *estate,
ResultRelInfo *resultRelInfo,
- ItemPointer tupleid);
+ ItemPointer tupleid,
+ TupleTableSlot *newSlot);
+static void ExecForPortionOfSaveRange(ModifyTableContext *context,
+ ResultRelInfo *resultRelInfo,
+ TupleTableSlot *slot);
static TupleTableSlot *ExecPrepareTupleRouting(ModifyTableState *mtstate,
EState *estate,
PartitionTupleRouting *proute,
@@ -1403,14 +1407,14 @@ static void
ExecForPortionOfLeftovers(ModifyTableContext *context,
EState *estate,
ResultRelInfo *resultRelInfo,
- ItemPointer tupleid)
+ ItemPointer tupleid,
+ TupleTableSlot *newSlot)
{
ModifyTableState *mtstate = context->mtstate;
ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
ForPortionOfExpr *forPortionOf = (ForPortionOfExpr *) node->forPortionOf;
AttrNumber rangeAttno;
Datum oldRange;
- TypeCacheEntry *typcache;
ForPortionOfState *fpoState;
TupleTableSlot *oldtupleSlot;
TupleTableSlot *leftoverSlot;
@@ -1490,15 +1494,51 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
oldRange = oldtupleSlot->tts_values[rangeAttno - 1];
/*
- * Get the range's type cache entry. This is worth caching for the whole
- * UPDATE/DELETE as range functions do.
+ * If BEFORE UPDATE triggers fired, they might have changed the range
+ * column, which would break the temporal semantics of FOR PORTION OF.
+ * We captured the column value in ExecForPortionOfSaveRange, so now
+ * compare it with the current value to detect tampering. This parallels
+ * how in analysis we reject SETting the range column directly.
*/
-
- typcache = fpoState->fp_leftoverstypcache;
- if (typcache == NULL)
+ if (newSlot != NULL && fpoState->fp_origNewRangeValid)
{
- typcache = lookup_type_cache(forPortionOf->rangeType, 0);
- fpoState->fp_leftoverstypcache = typcache;
+ bool newIsNull;
+ Datum newRange;
+ TypeCacheEntry *typcache;
+
+ /*
+ * Get the range's type cache entry. This is worth caching for the whole
+ * UPDATE/DELETE as range functions do.
+ */
+
+ typcache = fpoState->fp_leftoverstypcache;
+ if (typcache == NULL)
+ {
+ typcache = lookup_type_cache(forPortionOf->rangeType,
+ TYPECACHE_EQ_OPR_FINFO);
+ fpoState->fp_leftoverstypcache = typcache;
+ }
+
+ slot_getallattrs(newSlot);
+ newIsNull = newSlot->tts_isnull[rangeAttno - 1];
+ newRange = newSlot->tts_values[rangeAttno - 1];
+
+ if (!OidIsValid(typcache->eq_opr_finfo.fn_oid))
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_FUNCTION),
+ errmsg("could not identify an equality operator for type %s",
+ format_type_be(forPortionOf->rangeType)));
+
+ if (newIsNull != fpoState->fp_origNewRangeIsNull ||
+ (!newIsNull &&
+ !DatumGetBool(FunctionCall2Coll(&typcache->eq_opr_finfo,
+ InvalidOid,
+ newRange,
+ fpoState->fp_origNewRange))))
+ ereport(ERROR,
+ errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
+ errmsg("cannot change column \"%s\" from a BEFORE trigger because it is used in FOR PORTION OF",
+ forPortionOf->range_name));
}
/*
@@ -1617,6 +1657,92 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
}
}
+/* ----------------------------------------------------------------
+ * ExecForPortionOfSaveRange
+ *
+ * Capture the FOR PORTION OF range column value from the new tuple
+ * slot just before BEFORE UPDATE triggers run. ExecForPortionOfLeftovers
+ * later compares the saved value with the post-trigger value to detect
+ * whether a trigger changed the range column, which is not allowed.
+ * ----------------------------------------------------------------
+ */
+static void
+ExecForPortionOfSaveRange(ModifyTableContext *context,
+ ResultRelInfo *resultRelInfo,
+ TupleTableSlot *slot)
+{
+ ModifyTableState *mtstate = context->mtstate;
+ ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
+ ForPortionOfExpr *forPortionOf = (ForPortionOfExpr *) node->forPortionOf;
+ ForPortionOfState *fpoState;
+ AttrNumber rangeAttno;
+ TupleConversionMap *map = NULL;
+ MemoryContext oldcontext;
+
+ /*
+ * Lazily initialize the partition child's ForPortionOfState, mirroring
+ * ExecForPortionOfLeftovers so the saved value lives on the same struct
+ * the check will read from.
+ */
+ if (!resultRelInfo->ri_forPortionOf)
+ {
+ ForPortionOfState *leafState = makeNode(ForPortionOfState);
+ ForPortionOfState *rootFpoState;
+
+ if (!mtstate->rootResultRelInfo)
+ elog(ERROR, "no root relation but ri_forPortionOf is uninitialized");
+ rootFpoState = mtstate->rootResultRelInfo->ri_forPortionOf;
+ Assert(rootFpoState);
+
+ leafState->fp_rangeName = rootFpoState->fp_rangeName;
+ leafState->fp_rangeType = rootFpoState->fp_rangeType;
+ leafState->fp_rangeAttno = rootFpoState->fp_rangeAttno;
+ leafState->fp_targetRange = rootFpoState->fp_targetRange;
+ leafState->fp_Leftover = rootFpoState->fp_Leftover;
+ leafState->fp_Existing =
+ table_slot_create(resultRelInfo->ri_RelationDesc,
+ &mtstate->ps.state->es_tupleTable);
+
+ resultRelInfo->ri_forPortionOf = leafState;
+ }
+ fpoState = resultRelInfo->ri_forPortionOf;
+
+ rangeAttno = forPortionOf->rangeVar->varattno;
+ if (resultRelInfo->ri_RootResultRelInfo)
+ map = ExecGetChildToRootMap(resultRelInfo);
+ if (map != NULL)
+ rangeAttno = map->attrMap->attnums[rangeAttno - 1];
+
+ slot_getallattrs(slot);
+
+ /* Release any value saved from a prior row. */
+ if (fpoState->fp_origNewRangeValid)
+ {
+ fpoState->fp_origNewRangeValid = false;
+ if (!fpoState->fp_origNewRangeIsNull)
+ pfree(DatumGetPointer(fpoState->fp_origNewRange));
+ }
+
+ if (slot->tts_isnull[rangeAttno - 1])
+ {
+ fpoState->fp_origNewRange = (Datum) 0;
+ fpoState->fp_origNewRangeIsNull = true;
+ }
+ else
+ {
+ /*
+ * Make sure we copy everything for pass-by-reference types
+ * (like range and multirange).
+ */
+ oldcontext = MemoryContextSwitchTo(mtstate->ps.state->es_query_cxt);
+ fpoState->fp_origNewRange = datumCopy(slot->tts_values[rangeAttno - 1],
+ false, -1);
+ MemoryContextSwitchTo(oldcontext);
+ fpoState->fp_origNewRangeIsNull = false;
+ }
+ fpoState->fp_origNewRangeValid = true;
+}
+
/* ----------------------------------------------------------------
* ExecBatchInsert
*
@@ -1810,7 +1936,8 @@ ExecDeleteEpilogue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
/* Compute temporal leftovers in FOR PORTION OF */
if (((ModifyTable *) context->mtstate->ps.plan)->forPortionOf)
- ExecForPortionOfLeftovers(context, estate, resultRelInfo, tupleid);
+ ExecForPortionOfLeftovers(context, estate, resultRelInfo, tupleid,
+ NULL);
/* AFTER ROW DELETE Triggers */
ExecARDeleteTriggers(estate, resultRelInfo, tupleid, oldtuple,
@@ -2390,6 +2517,13 @@ ExecUpdatePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
if (context->estate->es_insert_pending_result_relations != NIL)
ExecPendingInserts(context->estate);
+ /*
+ * For FOR PORTION OF, remember the range column value so we can
+ * later detect whether a BEFORE trigger changed it.
+ */
+ if (((ModifyTable *) context->mtstate->ps.plan)->forPortionOf)
+ ExecForPortionOfSaveRange(context, resultRelInfo, slot);
+
return ExecBRUpdateTriggers(context->estate, context->epqstate,
resultRelInfo, tupleid, oldtuple, slot,
result, &context->tmfd,
@@ -2615,7 +2749,8 @@ ExecUpdateEpilogue(ModifyTableContext *context, UpdateContext *updateCxt,
/* Compute temporal leftovers in FOR PORTION OF */
if (((ModifyTable *) context->mtstate->ps.plan)->forPortionOf)
- ExecForPortionOfLeftovers(context, context->estate, resultRelInfo, tupleid);
+ ExecForPortionOfLeftovers(context, context->estate, resultRelInfo,
+ tupleid, slot);
/* AFTER ROW UPDATE Triggers */
ExecARUpdateTriggers(context->estate, resultRelInfo,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 13359180d25..efcd52411ab 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -483,6 +483,11 @@ typedef struct ForPortionOfState
TypeCacheEntry *fp_leftoverstypcache; /* type cache entry of the range */
TupleTableSlot *fp_Existing; /* slot to store old tuple */
TupleTableSlot *fp_Leftover; /* slot to store leftover */
+ Datum fp_origNewRange; /* range column value captured just before
+ * BEFORE UPDATE triggers fire, so we can
+ * detect whether they changed it */
+ bool fp_origNewRangeIsNull;
+ bool fp_origNewRangeValid; /* is fp_origNewRange meaningful? */
} ForPortionOfState;
/*
diff --git a/src/test/regress/expected/for_portion_of.out b/src/test/regress/expected/for_portion_of.out
index 31f772c723d..f9b9c1d0d6d 100644
--- a/src/test/regress/expected/for_portion_of.out
+++ b/src/test/regress/expected/for_portion_of.out
@@ -1793,6 +1793,26 @@ SELECT * FROM for_portion_of_test WHERE id = '[4,5)' ORDER BY id, valid_at;
(3 rows)
DROP TRIGGER fpo_after_delete_row ON for_portion_of_test;
+-- A BEFORE UPDATE trigger that changes the application-time column must
+-- raise an error, just as an explicit SET on that column does.
+CREATE FUNCTION trg_fpo_change_valid_at()
+RETURNS TRIGGER LANGUAGE plpgsql AS
+$$
+BEGIN
+ NEW.valid_at = daterange('2018-01-01', '2019-01-01');
+ RETURN NEW;
+END;
+$$;
+CREATE TRIGGER fpo_before_update_row
+ BEFORE UPDATE ON for_portion_of_test
+ FOR EACH ROW EXECUTE PROCEDURE trg_fpo_change_valid_at();
+UPDATE for_portion_of_test
+ FOR PORTION OF valid_at FROM '2018-05-01' TO '2018-06-01'
+ SET name = CONCAT(name, '!')
+ WHERE id = '[1,2)';
+ERROR: cannot change column "valid_at" from a BEFORE trigger because it is used in FOR PORTION OF
+DROP TRIGGER fpo_before_update_row ON for_portion_of_test;
+DROP FUNCTION trg_fpo_change_valid_at();
-- Test with multiranges
CREATE TABLE for_portion_of_test2 (
id int4range NOT NULL,
diff --git a/src/test/regress/sql/for_portion_of.sql b/src/test/regress/sql/for_portion_of.sql
index d4062acf1d1..39bb17a9409 100644
--- a/src/test/regress/sql/for_portion_of.sql
+++ b/src/test/regress/sql/for_portion_of.sql
@@ -1169,6 +1169,30 @@ SELECT * FROM for_portion_of_test WHERE id = '[4,5)' ORDER BY id, valid_at;
DROP TRIGGER fpo_after_delete_row ON for_portion_of_test;
+-- A BEFORE UPDATE trigger that changes the application-time column must
+-- raise an error, just as an explicit SET on that column does.
+
+CREATE FUNCTION trg_fpo_change_valid_at()
+RETURNS TRIGGER LANGUAGE plpgsql AS
+$$
+BEGIN
+ NEW.valid_at = daterange('2018-01-01', '2019-01-01');
+ RETURN NEW;
+END;
+$$;
+
+CREATE TRIGGER fpo_before_update_row
+ BEFORE UPDATE ON for_portion_of_test
+ FOR EACH ROW EXECUTE PROCEDURE trg_fpo_change_valid_at();
+
+UPDATE for_portion_of_test
+ FOR PORTION OF valid_at FROM '2018-05-01' TO '2018-06-01'
+ SET name = CONCAT(name, '!')
+ WHERE id = '[1,2)';
+
+DROP TRIGGER fpo_before_update_row ON for_portion_of_test;
+DROP FUNCTION trg_fpo_change_valid_at();
+
-- Test with multiranges
CREATE TABLE for_portion_of_test2 (
--
2.47.3
view thread (54+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: SQL:2011 Application Time Update & Delete
In-Reply-To: <CA+renyU5=mihx6O8+ERBmarZcuV1QiWB3X5bZhdptWoRM9G-Aw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox