Re: SQL:2011 Application Time Update & Delete

On Thu, Nov 27, 2025 at 7:44 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> Review of v62-0001-Document-temporal-update-delete.patch:

Thanks for the review! Here are v63 patches addressing your feedback,
plus some other things.

> This patch could be included in 0002 or placed after it, because it
> would not be applicable before committing 0002.

Okay, merged into one patch. The other one had some references to the
glossary entry here, so it can't come earlier.

> As in the previous patches you submitted that had images, the source
> .txt starts with empty lines that appear as extra top padding in the
> output. That should be removed.

Removed.

> Review of v62-0002-Add-UPDATE-DELETE-FOR-PORTION-OF.patch:
>
> 1) doc/src/sgml/ref/delete.sgml, doc/src/sgml/ref/update.sgml
>
> The use of "range_name" in the synopsis confused me for a while. I
> was thinking terms of range variables. Maybe range_column_name would
> be better.

Changed.

> The word "interval" is used here, but not in the usual SQL sense.
> Let's be careful about that. Maybe "range" or, well, "portion" would
> be better.

Okay.

> Also, there is some use of the word "history", but that's not a
> defined term here. Maybe that could be written differently to avoid
> that.

I replaced most cases of "history" with "application time". I think it
is a nice word to use though: concise, clear, and not jargony. I think
in the remaining case it is pretty clear it's a synonym.

Note that in ddl.sgml and dml.sgml I use "history" quite a bit to
explain what application time is all about.

> The syntactic details of what for_portion_of_target is should be in
> the synopsis. It could be broken out, like "where
> for_portion_of_target is" etc.

Done.

> start_time/end_time is described as "value", but it's really an
> expression. I don't see any treatment anywhere what kinds of
> expressions are allowed. Your commit message says NOW() is allowed,
> but how is that enforced? I would have expected to see a call to
> contain_volatile_functions() perhaps. I don't see any relevant tests.
> (At least if we're claiming NOW() is allowed, it should be in a test.)

With EXPR_KIND_FOR_PORTION we can forbid a lot of things. I was not
forbidding volatile functions though, so I added a check for that.
Testing with NOW() is tricky. I took some inspiration from this clever
trick, used in expression.sql: `SELECT current_timestamp = NOW()`. I
went for something similar, where the test calls the function but
avoids printing the timestamp itself. The tests now show that
current_date is allowed while clock_timestamp is not.

> The documentation writes that temporal leftovers are included in the
> returned count. I don't think this patches the SQL standard.
> Consider subclause <get diagnostics statement>, under ROW_COUNT it
> says:
>
> """
> Otherwise, let SC be the <search condition> directly contained in
> S. If <correlation name> is specified, then let MCN be “AS
> <correlation name>”; otherwise, let MCN be the zero-length character
> string. The value of ROW_COUNT is effectively derived by executing the
> statement:
>
> SELECT COUNT(*)
> FROM T MCN
> WHERE SC
>
> before the execution of S.
> """
>
> This means that the row count is determined by how many rows matched
> the search condition before the statement, not how many rows ended up
> after the statement.

Okay, fixed.

> 2) src/backend/parser/analyze.c
>
> addForPortionOfWhereConditions():
>
> It is not correct to augment the statement with artificial clauses at
> this stage. Most easily, this is evident if you reverse-compile the
> statement:
>
> CREATE FUNCTION foo() RETURNS text
> BEGIN ATOMIC
> UPDATE for_portion_of_test
> FOR PORTION OF valid_at FROM '2018-01-15' TO '2019-01-01'
> SET name = 'one^1' RETURNING name;
> END;
>
> \sf+ foo
> CREATE OR REPLACE FUNCTION public.foo()
> RETURNS text
> LANGUAGE sql
> 1 BEGIN ATOMIC
> 2 UPDATE for_portion_of_test SET name = 'one^1'::text
> 3 WHERE (for_portion_of_test.valid_at &&
> daterange('2018-01-15'::date, '2019-01-01'::date))
> 4 RETURNING for_portion_of_test.name;
> 5 END
>
> You can do these kinds of query modifications in the rewriter or
> later, because the stored node tree for a function, view, etc. is
> captured before that point. (For this particular case, either the
> rewriter or the optimizer might be an appropriate place, not sure.)

Okay, I thought it might be harmless for DML, so thanks for showing an
example where it matters. I moved this into the rewriter.

> Conversely, you need to do some work that the FOR PORTION OF clause
> gets printed back out when reverse-compiling an UPDATE statement.
> (See get_update_query_def() in ruleutils.c.) Add some tests, too.

Done.

> transformForPortionOfClause():
>
> Using get_typname_and_namespace() to get the name of a range type and
> then using that to construct a function call of the same name is
> fragile.
>
> Also, it leads to unexpected error messages when the types don't
> match:
>
> DELETE FROM for_portion_of_test
> FOR PORTION OF valid_at FROM 1 TO 2;
> ERROR: function pg_catalog.daterange(integer, integer) does not exist
>
> Well, you cover that in the tests, but I don't think it's right.
>
> There should be a way to go into the catalogs and get the correct
> range constructor function for a range type using only OID references.
> Then you can build a FuncExpr node directly and don't need to go the
> detour of building a fake FuncCall node to transform. (You'd still
> need to transform the arguments separately in that case.)

I added a function, get_range_constructor2, which I call to build a
FuncExpr now. I got rid of get_typname_and_namespace. That said,
looking up the constructor is tricky, because there isn't a direct oid
lookup you can make. The rule is that it has the same name as the
rangetype, with two args both matching the subtype. At least the rule
is encapsulated now. And I think this function will be useful for the
PERIODs patch, which needs similar don't-parse-your-own-node-trees
work.

I improved the error message as well, if the types don't match.

These patches include several other improvements & tests related to
type-checking the FOR PORTION OF target. In particular jian he's
recent finding about WITHOUT OVERLAPS lacking DOMAIN support [0] made
me realize I needed that here too.

> transformUpdateTargetList():
>
> The error message should provide a reason, like "cannot update column
> X because it is mentioned in FOR PORTION OF".

Okay.

> 3) src/backend/parser/gram.y
>
> I don't think there is a clear policy on that (maybe there should be),
> but I wouldn't put every single node type into the %union. Instead,
> declare the result type of a production as <node> and use a bit of
> casting.

Okay. I was following things like OnConflictClause, but I can see how
this makes the list unwieldy. Now the production just a Node.

> 4) src/backend/utils/adt/ri_triggers.c
>
> Is this comment change created by this patch or an existing situation?

You're right, it should be separate. Submitted elsewhere as its own patch.

> 5) src/include/nodes/parsenodes.h
>
> Similar to the documentation issue mentioned above, the comments for
> the ForPortionOfClause struct use somewhat inconsistent terminology.
> The comment says <period-name>, the field is range_name. Also <ts> vs
> target_start etc. hinders quick mental processing. The use of the
> word "target" in this context is also new.

Okay, I updated the comment to match the fields.

"target" is used in the syntax docs above for update & delete, and
also in dml.sgml. I think it's important to have a word for what
portion of history you want to change. I like "target" because it
accommodates both the FROM ... TO ... syntax and the (...) syntax, it
is concise and vivid, and it isn't ambiguous. Do you want me to add a
glossary entry for, say, "target, for portion of"?

> The location field should have type ParseLoc.

Okay.

> 6) src/include/parser/parse_node.h
>
> Somehow, the EXPR_KIND_UPDATE_PORTION switch cases all appear in
> different orders in different places. Could you arrange it so that
> there is some consistency there?

Fixed.

> Also, maybe name this so it does not give the impression that it does
> not apply to DELETE. Maybe EXPR_KIND_FOR_PORTION.

Changed.

> 7) src/test/regress/expected/for_portion_of.out,
> src/test/regress/sql/for_portion_of.sql
>
> There are several places where the SELECT statement after an UPDATE or
> DELETE statement is indented as if it were part of the previous
> statement. That is probably not intentional.

Fixed.

> For the first few tests, I would prefer to see a SELECT after each
> UPDATE or DELETE so you can see what each statement is doing
> separately.

Okay, done.

> There are tests about RETURNING behavior, but the expected behavior
> does not appear to be mentioned in the documentation.

Added.

> 8) src/test/regress/expected/privileges.out,
> src/test/regress/sql/privileges.sql
>
> This tests that UPDATE privilege on the range column is required. But
> I don't see this matching the SQL standard, and I also don't see why
> it would be needed, since you are not actually writing to that column.
> SELECT privilege of the column is required, because it becomes
> effectively part of the WHERE clause. That should be tested here.

You really don't need update permission? The columns do get updated. I
changed it, but it seems a little strange. On the other hand since you
don't need insert permission for leftovers, maybe it's consistent.

I added a check requiring select permission and updated the tests.

For the PERIODs patch (which is less ready than the rest and lower
priority to me), I'm still wrongly adding to updatedCols for now,
because it turns out that ExecInitGenerated won't update the generated
valid_at column otherwise, because it calls ExecGetUpdatedCols, which
looks in the perminfo. Maybe that is a misuse of the property that
needs to be improved first.

> 9) src/test/regress/expected/updatable_views.out,
> src/test/regress/sql/updatable_views.sql
>
> Add something like ORDER BY id, valid_at to the example queries here
> (similar to for_portion_of.sql). That makes them easier to understand
> and also more stable in execution.

Okay.

> 10) src/test/subscription/t/034_temporal.pl
>
> Many of these tests just fail because there is no replica identity
> set, and that's already tested with a plain UPDATE statement. The
> addition of FOR PORTION OF doesn't change that. Maybe we can drop
> most of these tests.

Okay. Replaced with a comment though, since there is a systematic
structure there I want to preserve.

> It might also be useful to add a few tests to contrib/test_decoding,
> to demonstrate on a logical-decoding level how a statement with FOR
> PORTION OF resolves into multiple different row events.

Done.

I also improved the executor where I was setting up a state object for
each partition in a partition tree. Now I do this lazily, so that you
don't pay for every partition if you are only changing one.

Rebased to 8f1791c6183.

[0] https://www.postgresql.org/message-id/CACJufxGoAmN_0iJ%3DhjTG0vGpOSOyy-vYyfE%2B-q0AWxrq2_p5XQ%40mail.gmail.com

Yours,

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