On 13.03.26 18:06, Paul A Jungwirth wrote:
3.7)
+-- UPDATE ... RETURNING returns only the updated values (not the
inserted side values)
This test looks redundant with earlier tests. Otherwise, maybe add a
comment about how it's different.
I don't think a top-level RETURNING test is redundant with the CTE
test. I expanded the comment here a bit to clarify the goal. It
addresses your question above: Should RETURNING include the inserted
leftovers? I don't think that makes sense:
1. Our docs say, "The optional RETURNING clause causes UPDATE to
compute and return value(s) based on each row actually updated." The
leftovers were not updated.
2. Conceptually, the leftovers represent what *didn't* change.
3. If you implemented this with a trigger, you also wouldn't get the
inserted leftovers.
4. The SQL standard doesn't have RETURNING. But it does say that to
insert the leftovers the system should execute a separate insert
"statement". So we should do something very similar to the trigger
case.
UPDATE ... RETURNING is in my mind equivalent to the SQL standard SELECT
... FROM NEW TABLE (UPDATE ...) (see <data change delta table>). So I
was hoping for an answer there, but it just says:
"If TP simply contains a <data change delta table> DCDT, then let S be
the <data change statement> simply contained in TP. S shall not contain
FOR PORTION OF."
So we can pick our own behavior. Your explanation makes sense. (I
suppose an alternative is that we also don't allow using FOR PORTION OF
together with RETURNING?)
5) NULL bounds
A general comment: In particular after studying these tests in detail,
I'm suspicious that it's a good idea to interpret null bounds as
unbounded. Expressions could return null for nested reasons, it would
be very hard to follow that. Null values should mean "unknown",
unbounded should be explicit. We have the keyword UNBOUNDED already,
maybe you could use that? Or do you want to be able to return
unboundedness from an expression?
I like the idea of a keyword. I tried adding UNBOUNDED but it caused a
few hundred S/R and R/R conflicts that I couldn't easily resolve. A
year or two ago I had keywords here (MINVALUE/MAXVALUE IIRC), but it
required some nasty parser hacks. This is a pretty delicate area of
the grammar, because we have a_expr with FROM and TO and no
punctuation. I'm already doing some contortions to handle `FOR PORTION
OF valid_at FROM t1 + INTERVAL '1' YEAR TO MONTH TO t2`.
A keyword is not offered by the standard here, so it would just be
custom syntactic sugar. No other RDBMS has one (I think).
I think NULL is the right choice for unbounded. It is what range types
use, and we want this to mesh well with them. More important it works
for *any type*. We don't always have +/-Infinity.
Also I think we should expand user choice rather than restrict it. If
users want to forbid nulls, they can (e.g. by using a domain type).
But if we forbid it, there is no way to override that decision.
Going back to the UNBOUNDED keyword: if we forbid nulls, then a
keyword doesn't really add clarity, since users would already say
`-Infinity` or `Infinity`. It's really just a way to express what null
means in this context. Assuming we keep nulls, I'd like to keep
working on a keyword. But I think we could add it later.
Yeah, this seems like something we could change later with relative
ease. Maybe solicit some input from the public during beta?
Btw what do you think of the READ COMMITTED issues I brought up in my
third patch? We follow MariaDB here, but not DB2. DB2's behavior is
less problematic for users, although their isolation levels don't
quite match ours. If we're not okay with those results, we should
address them before merging the main patch.
It's still hard to understand. I would be ok in general to say that
results might be unexpected unless you use REPEATABLE READ. Especially
as it seems that a technical solution to improve this would be possible
later. But we should document this in more detail. The verbal
explanations are hard to interpret. Could you maybe come up with a
couple of ASCII-art flow charts that explains how things could go
strange that we could put into the documentation?
Could we actually put some of these strange/unexpected behaviors into
the isolation tests? Right now we only test that the workaround works
but not the initial problem. Is this possible? (Would we need
injection points?)
Could we cut back the isolation tests a bit? They are the second slowest
isolation test now, and the second largest expected file. Maybe we
don't need to test SERIALIZE separately? (Assume that SERIALIZE is as
good as or better than REPEATABLE READ?)
Attached is a patch with a few small cosmetic corrections. In
ExecForPortionOfLeftovers(), the comment and code that I delete is
duplicated before and inside the loop. The one before the
loop is probably sufficient.
Other than all that, this patch set (0001 through 0003) seems good to me.
From eb73e1971d0fd05b6d66c3b79552b396fc5f3b22 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Wed, 25 Mar 2026 16:42:18 +0100
Subject: [PATCH] Fixups
---
src/backend/executor/nodeModifyTable.c | 9 +--------
src/backend/parser/analyze.c | 3 +--
src/include/nodes/execnodes.h | 1 +
src/test/regress/expected/for_portion_of.out | 18 +++++++++---------
src/test/regress/sql/for_portion_of.sql | 18 +++++++++---------
5 files changed, 21 insertions(+), 28 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index e16299cc2ed..9324bb1a093 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -135,6 +135,7 @@ typedef struct UpdateContext
LockTupleMode lockmode;
} UpdateContext;
+
static void ExecBatchInsert(ModifyTableState *mtstate,
ResultRelInfo
*resultRelInfo,
TupleTableSlot **slots,
@@ -1588,14 +1589,6 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
leftoverSlot->tts_isnull[forPortionOf->rangeVar->varattno - 1]
= false;
ExecMaterializeSlot(leftoverSlot);
- /*
- * If there are partitions, we must insert into the root table,
so we
- * get tuple routing. We already set up leftoverSlot with the
root
- * tuple descriptor.
- */
- if (resultRelInfo->ri_RootResultRelInfo)
- resultRelInfo = resultRelInfo->ri_RootResultRelInfo;
-
/*
* The standard says that each temporal leftover should execute
its
* own INSERT statement, firing all statement and row triggers,
but
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index f0e9ebaddd5..9e9b7fb18d2 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1348,8 +1348,7 @@ transformForPortionOfClause(ParseState *pstate,
attbasetype = getBaseType(attr->atttypid);
- rangeVar = makeVar(
- rtindex,
+ rangeVar = makeVar(rtindex,
range_attno,
attr->atttypid,
attr->atttypmod,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 177ded4e5cd..090cfccf65f 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -41,6 +41,7 @@
#include "utils/reltrigger.h"
#include "utils/typcache.h"
+
/*
* forward references in this file
*/
diff --git a/src/test/regress/expected/for_portion_of.out
b/src/test/regress/expected/for_portion_of.out
index 8a29a19f501..31f772c723d 100644
--- a/src/test/regress/expected/for_portion_of.out
+++ b/src/test/regress/expected/for_portion_of.out
@@ -1636,17 +1636,17 @@ CREATE TABLE for_portion_of_test (
INSERT INTO for_portion_of_test (id, valid_at, name) VALUES
('[1,2)', '[2018-01-01,2020-01-01)', 'one');
CREATE CONSTRAINT TRIGGER fpo_after_insert_row
-AFTER INSERT ON for_portion_of_test
-DEFERRABLE INITIALLY DEFERRED
-FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
+ AFTER INSERT ON for_portion_of_test
+ DEFERRABLE INITIALLY DEFERRED
+ FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
CREATE CONSTRAINT TRIGGER fpo_after_update_row
-AFTER UPDATE ON for_portion_of_test
-DEFERRABLE INITIALLY DEFERRED
-FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
+ AFTER UPDATE ON for_portion_of_test
+ DEFERRABLE INITIALLY DEFERRED
+ FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
CREATE CONSTRAINT TRIGGER fpo_after_delete_row
-AFTER DELETE ON for_portion_of_test
-DEFERRABLE INITIALLY DEFERRED
-FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
+ AFTER DELETE ON for_portion_of_test
+ DEFERRABLE INITIALLY DEFERRED
+ FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
BEGIN;
UPDATE for_portion_of_test
FOR PORTION OF valid_at FROM '2018-01-15' TO '2019-01-01'
diff --git a/src/test/regress/sql/for_portion_of.sql
b/src/test/regress/sql/for_portion_of.sql
index 20d7e879c14..d4062acf1d1 100644
--- a/src/test/regress/sql/for_portion_of.sql
+++ b/src/test/regress/sql/for_portion_of.sql
@@ -1037,19 +1037,19 @@ CREATE TABLE for_portion_of_test (
('[1,2)', '[2018-01-01,2020-01-01)', 'one');
CREATE CONSTRAINT TRIGGER fpo_after_insert_row
-AFTER INSERT ON for_portion_of_test
-DEFERRABLE INITIALLY DEFERRED
-FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
+ AFTER INSERT ON for_portion_of_test
+ DEFERRABLE INITIALLY DEFERRED
+ FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
CREATE CONSTRAINT TRIGGER fpo_after_update_row
-AFTER UPDATE ON for_portion_of_test
-DEFERRABLE INITIALLY DEFERRED
-FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
+ AFTER UPDATE ON for_portion_of_test
+ DEFERRABLE INITIALLY DEFERRED
+ FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
CREATE CONSTRAINT TRIGGER fpo_after_delete_row
-AFTER DELETE ON for_portion_of_test
-DEFERRABLE INITIALLY DEFERRED
-FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
+ AFTER DELETE ON for_portion_of_test
+ DEFERRABLE INITIALLY DEFERRED
+ FOR EACH ROW EXECUTE PROCEDURE dump_trigger(false, false);
BEGIN;
UPDATE for_portion_of_test
--
2.53.0
