Feedback-ID: ie0a040ee:Fastmail
Message-ID: <85ac7f0e-d95f-4377-ade0-8941fd328012@eisentraut.org>
Date: Thu, 27 Nov 2025 16:44:26 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: SQL:2011 Application Time Update & Delete
To: Paul A Jungwirth <pj@illuminatedcomputing.com>
Cc: Chao Li <li.evan.chao@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <ec498c3d-5f2b-48ec-b989-5561c8aa2024@illuminatedcomputing.com>
 <CA+renyWA-b00qvz4gDdPbPBjVatacB1T5v7SJc6J_xg3R6-qRw@mail.gmail.com>
 <CA+renyVYnV9rGDiUhTQEy8r6gx6Xg-+OEo-2DK7JbuBXcLVq5A@mail.gmail.com>
 <CA+renyWcNBdnaW4zc9S03aN+fEbVVB1S+q8e9MEjhM2YB+kkiw@mail.gmail.com>
 <CA+renyUiEu2yTHk+Vy-Rt2xA5Vnp2N_ohN=npiNmMKTh53qDfA@mail.gmail.com>
 <2f5364f3-a1d3-4410-98f3-d788b11e6525@eisentraut.org>
 <CA+renyW7ZB_k9AgmSFJU2EegL9r1k1sgWo4-9tGGkgwxNqe6kw@mail.gmail.com>
 <CA+renyUodzxAvMnpa_LTvo+Ru1ZKH+Su8KaPvD4iMtguFKzq4g@mail.gmail.com>
 <1ace7bc1-9dd4-42c9-a473-517cef37cce9@eisentraut.org>
 <CA+renyU-iz_zvM0gGP=dvBPVrz=Jj3qdCjtAh5nLZRhb49xMFw@mail.gmail.com>
 <6F8D7105-BD1C-432D-84F3-BC688C0C8EDC@gmail.com>
 <9B820A52-D2F6-465D-B258-6FE8EBA59FAE@gmail.com>
 <CA+renyXchHgpMbYX-cR8fuNnnpf_+FJ6PkHoXoa2AgzRnz4vxQ@mail.gmail.com>
 <53a13f97-340f-4d04-9dcc-77ca3ffb6a6a@eisentraut.org>
 <CA+renyXH3AF6JVzZGVcT5mAo=0QncB-MpWJeqb2JG66sgyq09g@mail.gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: 
 <CA+renyXH3AF6JVzZGVcT5mAo=0QncB-MpWJeqb2JG66sgyq09g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/85ac7f0e-d95f-4377-ade0-8941fd328012%40eisentraut.org>
Precedence: bulk

On 26.11.25 20:29, Paul A Jungwirth wrote:
> On Sat, Nov 22, 2025 at 12:55 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> On 19.11.25 19:49, Paul A Jungwirth wrote:
>>> On Thu, Nov 13, 2025 at 8:10 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>>> I continue reviewing ...
>>>
>>> Thank you for another detailed review! New patches are attached (v61),
>>> details below.
>>
>> I have committed 0001 and 0003 from this set.  I will continue reviewing
>> the rest.
> 
> Thanks! Rebased to e135e04457.

Review of v62-0001-Document-temporal-update-delete.patch:

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

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.


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.

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.

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.

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.

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

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.


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

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.

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

transformUpdateTargetList():

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


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.


4) src/backend/utils/adt/ri_triggers.c

Is this comment change created by this patch or an existing situation?


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.

The location field should have type ParseLoc.


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?

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


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.

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.

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


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.


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.


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.

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.