Re: SQL:2011 Application Time Update & Delete

On Thu, Mar 12, 2026 at 1:39 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> Hi Paul,
>
> Review of v67-0001-Add-range_get_constructor2-to-lsyscache.patch and
> v67-0002-Add-UPDATE-DELETE-FOR-PORTION-OF.patch:

Thanks for taking another look! v68 patches attached, details below:

> 1) In src/backend/parser/analyze.c, transformForPortionOfClause():
>
> The variable range_name is not very useful; using
> forPortionOf->range_name is clearer.

Okay, done.

> Try making the forPortionOf argument const.

Done.

> The initialization strat = RTOverlapStrategyNumber; is confusing.

You're right. That was left over from when GetOperatorFromCompareType
took an in/out param. Now it's purely out.

> 2) src/backend/parser/gram.y
>
> Explain the %prec in opt_alias with a comment. Maybe the production
> name should be more specific if it's only applicable to some specific
> statement.

Added an explanation and renamed the production.

> 3) In the test src/test/regress/expected/for_portion_of.out:
>
> 3.1)
>
> Fix the wording of the error message here (remove "period", add back in
> later patch):
>
> +ERROR: column or period "invalid_at" of relation "for_portion_of_test"
> does not exist

Done. I've added a commit to restore "or period" to my private PERIOD
branch, but it's not part of this series anymore. I'll introduce it
later.

> 3.2)
>
> Could this error message have a location pointer?
>
> +ERROR: range lower bound must be less than or equal to range upper bound

This is harder than I thought. It happens inside
contain_volatile_functions_after_planning. Even if I omit that call
entirely, it still happens during planning from
eval_const_expressions. Is there some way to pass a location down that
far, e.g. with soft error context? I don't see how to do it.

> 3.3)
>
> Improve wording of this error message:
>
> +ERROR: got a NULL FOR PORTION OF target
>
> ("FOR PORTION OF target was null"?)

Changed to your suggestion. Also this should be an ereport, not elog,
since the (...) syntax would let a user give a NULL directly. Also I
added a location pointer.

> 3.4)
>
> +-- Updating the non-range part of the PK:
>
> This test updates the id column but the SELECT afterwards only shows
> rows for the old id value. The SELECT ought to use something like
>
> WHERE id = '[1,2)' OR id = '[6,7)'

Okay, done.

> 3.5)
>
> -- UPDATE FOR PORTION OF in a CTE:
>
> I think this test is meant to show that the UPDATE FOR PORTION OF
> ... RETURNING returns only the updated rows, not the inserted
> "leftovers".
>
> First, it would be good if the comment was more clear about what it's
> trying to demonstrate.
>
> And then, is there a reason for this behavior versus the alternatives?
> SQL standard, other implementations?

My goal here wasn't to test RETURNING per se, but just to see if FOR
PORTION OF composed with CTEs properly. (There are a bunch of small
tests in this area of the file testing composition with other
features.) But a CTE has to return *something*, so I had to put
RETURNING there. I was especially interested in MVCC visibility, both
of the update changes (including automatically updating valid_at) and
the leftover inserts. I added a comment to the test file with all
that.

As you say, there is another test below that focuses more on RETURNING
(as a top-level statement, not inside a CTE). I'll address your
questions about RETURNING behavior down there.

> 3.6)
>
> +-- Not visible to UPDATE:
> +-- Tuples updated/inserted within the CTE are not visible to the main
> query yet,
> +-- but neither are old tuples the CTE changed:
>
> Is this behavior the same or different from the way normal queries
> work? Could be clarified in the comment either way.

I expanded the comment. It's the same as how other queries work.

> 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.

5. I tried comparing our behavior to MariaDB and IBM DB2. MariaDB
doesn't have RETURNING, so it's no help. DB2 has RETURNING INTO inside
PL/SQL, but I couldn't get it to work. If I do, I'll update this
thread.

6. Omitting the leftovers is consistent with what we're doing for
firing update/insert row/statement triggers and what we're putting in
the transition trigger tables.

7. If we included leftovers in UPDATE RETURNING, we should include
them in DELETE RETURNING, and that makes even less sense.

Personally, as a user of this feature, getting the leftovers back from
RETURNING would be very unexpected and annoying. So I think we are
doing the right thing here.

> 3.8)
>
> +-- test that we run triggers on the UPDATE/DELETEd row and the INSERTed
> rows
>
> Please indent the non-first rows of the CREATE TRIGGER statements.

Done.

> 4) In src/test/subscription/t/034_temporal.pl
>
> +[4,5)|[2000-01-01,2010-01-01)|a}, 'replicated temporal_unique DEFAULT');
>
> The change from FULL to DEFAULT seems wrong.

You're right; fixed.

> 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.

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.

Rebased to 1c33a2d81d.

Yours,

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com