Обсуждение: SQL:2011 Application Time Update & Delete

Hi Hackers,

Here is a new thread for the next part of SQL:2011 Application Time: UPDATE and DELETE commands with 
FOR PORTION OF. This continues the long-running thread that ended with [1].

I don't have a new patch set yet, but I wanted to summarize the discussion at the PGConf.dev 
Advanced Patch Feedback session, especially to continue the conversation about triggers fired from 
inserting "temporal leftovers" as part of an UPDATE/DELETE FOR PORTION OF.

In my last patch series, I fire all statement & row triggers when the inserts happen for temporal 
leftovers. So let's assume there is a row with valid_at of [2000-01-01,2020-01-01) and the user's 
query is UPDATE t FOR PORTION OF valid_at FROM '2010-01-01' TO '2011-01-01'. So it changes one row, 
targeting only 2010. There are two temporal leftovers: one for 2000-2009 and one for 2011-2019 
(inclusive). Then these triggers fire in the order given:

BEFORE UPDATE STATEMENT
BEFORE UPDATE ROW
BEFORE INSERT STATEMENT -- for the 2000-2009 leftovers
BEFORE INSERT ROW
AFTER INSERT ROW
AFTER INSERT STATEMENT
BEFORE INSERT STATEMENT -- for the 2011-2019 leftovers
BEFORE INSERT ROW
AFTER INSERT ROW
AFTER INSERT STATEMENT
AFTER UPDATE ROW
AFTER UPDATE STATEMENT

I think this is the correct behavior (as I'll get to below), but at the session none of us seemed 
completely sure. What we all agreed on is that we shouldn't implement it with SPI.

Before I switched to SPI, I feared that getting INSERT STATEMENT triggers to fire was going to cause 
a lot of code duplication. But I took my last pre-SPI patch (v39 from 7 Aug 2024), restored its 
implementation for ExecForPortionOfLeftovers, and got the desired behavior with just these lines 
(executed once per temporal leftover):

AfterTriggerBeginQuery()
ExecSetupTransitionCaptureState(mtstate, estate);
fireBSTriggers(mtstate);
ExecInsert(context, resultRelInfo, leftoverSlot, node->canSetTag, NULL, NULL);
fireASTriggers(mtstate);
AfterTriggerEndQuery(estate);

You'll be able to see all that with my next patch set, but for now I'm just saying: replacing SPI 
was easier than I thought.

There were different opinions about whether this behavior is correct. Robert and Tom both thought 
that firing INSERT STATEMENT triggers was weird. (Please correct me if I misrepresent anything you 
said!)

Robert pointed out that if you are using statement triggers for performance reasons (since that may 
be the only reason to prefer them to row triggers), you might be annoyed to find that your INSERT 
STATEMENT triggers fire up to two times every time you update a *row*.

Robert also warned that some people implement replication with statement triggers (though maybe not 
people running v18), and they might not like INSERT STATEMENT triggers firing when there was no 
user-issued insert statement. This is especially true since C-based triggers have access to the FOR 
PORTION OF details, as do PL/pgSQL triggers (in a follow-on patch), so they don't need to hear about 
the implicit inserts.

Also trigger-based auditing will see insert statements that were never explicitly sent by a user.
(OTOH this is also true for inserts made from triggers, and (as we'll see below) several other 
commands fire statement triggers for implicit actions.)

Robert & Tom agreed that if we leave out the statement triggers, then the NEW transition table for 
the overall UPDATE STATEMENT trigger should include all three rows: the updated version of the old 
row and the (up to) two temporal leftovers.

A philosophical argument I can see for omitting INSERT STATEMENT is that the temporal leftovers only 
preserve the history that was already there. They don't add to what is asserted by the table. But 
reporting them as statements feels a bit like treating them as user assertions. (I'm not saying I 
find this argument very strong, but I can see how someone would make it.)

Tom & Robert thought that firing the INSERT *ROW* triggers made sense and was valuable for some 
use-cases, e.g. auditing.

Robert also thought that nesting was weird. He thought that the order should be this (and even 
better if omitting the INSERT STATEMENTs):

BEFORE UPDATE STATEMENT
BEFORE UPDATE ROW
AFTER UPDATE ROW
AFTER UPDATE STATEMENT
BEFORE INSERT STATEMENT -- for the 2000-2009 leftovers
BEFORE INSERT ROW
AFTER INSERT ROW
AFTER INSERT STATEMENT
BEFORE INSERT STATEMENT -- for the 2011-2019 leftovers
BEFORE INSERT ROW
AFTER INSERT ROW
AFTER INSERT STATEMENT

But I think that the behavior I have is correct. My draft copy of the 2011 standard says this about 
inserting temporal leftovers (15.13, General Rules 10.c.ii):

 > The following <insert statement> is effectively executed without further Access Rule
 > and constraint checking:
 >   INSERT INTO TN VALUES (VL1, ..., VLd)

When I compared IBM DB2 and MariaDB, I found that DB2 does this:

AFTER INSERT ROW -- for the 2000-2009 leftovers
AFTER INSERT STATEMENT
AFTER INSERT ROW -- for the 2011-2019 leftovers
AFTER INSERT STATEMENT
AFTER UPDATE ROW
AFTER UPDATE STATEMENT

(I didn't quickly find a way to observe BEFORE triggers firing, so they aren't show here. I was 
misremembering when I said at the session that it doesn't support BEFORE triggers. It does, but they 
can't do certain things, like insert into an auditing table.)

And MariaDB (which doesn't have statement triggers) does this:

BEFORE UPDATE ROW
BEFORE INSERT ROW -- for the 2000-2009 leftovers
AFTER INSERT ROW
BEFORE INSERT ROW -- for the 2011-2019 leftovers
AFTER INSERT ROW
AFTER UPDATE ROW

So both of those match the behavior I've implemented (including the nesting).

Peter later looked up the current text of the standard, and he found several parts that confirm the 
existing behavior. (Thank you for checking that for me Peter!) To paraphrase a note from him:

Paper SQL-026R2, which originally created this feature, says:

 > All UPDATE triggers defined on the table will get activated in the usual way for all rows that are
 > updated. In addition, all INSERT triggers will get activated for all rows that are inserted.

He also found the same text I quoted above (now in section 15.14).

He also brought up this other passage from SQL-026R2:

 > Currently it is not possible
 > for the body of an UPDATE trigger to gain access to the FROM and TO values in the FOR PORTION OF
 > clause if one is specified. The syntax of <trigger definition> will need to be extended to allow
 > such access. We are not proposing to enhance the syntax of <trigger definition> in this proposal.
 > We leave it as a future Language Opportunity.

Since the standard still hasn't added that, firing at least INSERT ROW triggers is necessary if you 
want trigger-based replication. (I don't think this speaks strongly to INSERT STATEMENT triggers 
though.)

Incidentally, note that my patches *do* include this information (as noted above): both in the 
TriggerData struct passed to C triggers, and (in a separate patch) via PL/pgSQL variables. I don't 
include it for SQL-language triggers, and perhaps those should wait to see what the standard recommends.

In a world where we *do* fire statement triggers, I think each statement should get its own 
transition table contents.

Robert also said that we should choose behavior that is consistent with other features in Postgres.
I've attached a script to demonstrate a few interesting comparisons. It tests:

- INSERT ON CONFLICT DO NOTHING (without then with a conflict)
- INSERT ON CONFLICT DO UPDATE (without then with a conflict)
- INSERT ON CONFLICT DO UPDATE WHERE (with a conflict)
- MERGE DO NOTHING (without then with a conflict)
- MERGE UPDATE (without then with a conflict)
- cross-partition UPDATE
- ON DELETE CASCADE
- ON DELETE SET NULL

ON CONFLICT DO NOTHING and MERGE DO NOTHING do not fire an UPDATE STATEMENT trigger (naturally).

Cross-partition update does not fire extra statement triggers. Everything else does fire extra 
statement triggers. I think this is what I would have guessed if I hadn't tested it first. It feels 
like the natural choice for each feature.

Note that commands have to "decide" a priori which statement triggers they'll fire, before they 
process rows. So ON CONFLICT DO UPDATE fires first BEFORE INSERT STATEMENT, then BEFORE UPDATE 
STATEMENT, then row triggers, and finally AFTER UPDATE STATEMENT and AFTER INSERT STATEMENT. MERGE 
UPDATE is the same. It fires BEFORE INSERT STATEMENT, then BEFORE UPDATE STATEMENT, then row 
triggers, and finally AFTER UPDATE STATEMENT and AFTER INSERT STATEMENT. And the referential 
integrity actions fire statement triggers (as expected, since they are implemented with SPI).

In all cases we see nesting. With cross-partition update, the DELETE & INSERT triggers are nested 
inside the before/after UPDATE trigger (although interestingly the AFTER DELETE/INSERT triggers 
don't quite follow a nesting-like order with respect to each other):

BEFORE UPDATE STATEMENT
BEFORE UPDATE ROW
BEFORE DELETE ROW
BEFORE INSERT ROW
AFTER DELETE ROW
AFTER INSERT ROW
AFTER UPDATE STATEMENT

That covers all my research. My conclusion is that we *should* fire INSERT STATEMENT triggers, and 
they should be nested within the BEFORE & AFTER UPDATE triggers. I'm pleased that achieving that 
without SPI is not as hard as I expected.

Please stay tuned for some actual patches!

[1] 
https://www.postgresql.org/message-id/CA%2BrenyUZuWOxvY1Lv9O3F1LdpKc442EYvViR1DVzbD9ztaa6Yg%40mail.gmail.com

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

On Sun, Jun 22, 2025 at 6:19 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
> Here are updated patches for UPDATE/DELETE FOR PORTION OF and related
> functionality. I left out the usual PERIODs patch because I'm still
> updating it to work with the latest master.

Here is a new set of patches, rebased to 325fc0ab14. No material changes.

I'm still working on the PERIOD DDL, but that doesn't have to go in at
the same time. The tricky part is ALTER TABLE ADD PERIOD, where I need
to wait until the add-columns pass to see the start/end columns'
type/etc, but then in that same pass I need to add a generated range
column. If I add the column in a later pass, I get a failure, e.g.
"cannot ALTER TABLE "pt" because it is being used by active queries in
this session". This only appeared with recent(ish) NOT NULL work. I
think the solution is to avoid holding a relcache entry longer than
needed, but I haven't had a chance to locate the issue yet.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On Fri, Aug 29, 2025 at 6:03 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
> I'm still working on the PERIOD DDL, but that doesn't have to go in at
> the same time. The tricky part is ALTER TABLE ADD PERIOD, where I need
> to wait until the add-columns pass to see the start/end columns'
> type/etc, but then in that same pass I need to add a generated range
> column. If I add the column in a later pass, I get a failure, e.g.
> "cannot ALTER TABLE "pt" because it is being used by active queries in
> this session". This only appeared with recent(ish) NOT NULL work. I
> think the solution is to avoid holding a relcache entry longer than
> needed, but I haven't had a chance to locate the issue yet.

Here is another update, now with working PERIOD DDL. I also fixed some
new post-rebase problems causing CI to fail.

There is a detailed wiki page attached to the commitfest entry. To
summarize the patches here:

- Four documentation patches adding a new chapter introducing temporal
concepts. This are split out by topic: primary key + unique
constraints, foreign keys, PERIODs, and UPDATE/DELETE FOR PORTION OF.
- Two patches adding UPDATE/DELETE FOR PORTION OF. (I broke out the
helper functions that compute temporal leftovers.)
- Some patches adding CASCADE/SET NULL/SET DEFAULT to temporal foreign
keys. Once you have UPDATE/DELETE FOR PORTION OF, these are easy. You
do need to know the FOR PORTION OF bounds though, so one of the
patches adds that to the TriggerData struct.
- A patch to add the same bounds info to PL/pgSQL trigger variables.
- A patch to add PERIOD DDL support, based on hidden GENERATED
rangetype columns.

Rebased to d96c854dfc.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On Wed, Sep 24, 2025 at 9:05 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Here is another update, now with working PERIOD DDL. I also fixed some
> new post-rebase problems causing CI to fail.

More rebase & CI fixes attached.

Rebased to 03d40e4b52 now.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On Sat, Oct 4, 2025 at 12:48 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Wed, Sep 24, 2025 at 9:05 PM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > Here is another update, now with working PERIOD DDL. I also fixed some
> > new post-rebase problems causing CI to fail.
>
> More rebase & CI fixes attached.
>
> Rebased to 03d40e4b52 now.

It looks like an #include I needed went away and my patches stopped
compiling. Here is a new series.

Now rebased to 7a662a46eb.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On Sun, Oct 12, 2025 at 11:43 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
> > > Here is another update, now with working PERIOD DDL. I also fixed some
> > > new post-rebase problems causing CI to fail.
> >
> > More rebase & CI fixes attached.
> >
> > Rebased to 03d40e4b52 now.
>
> It looks like an #include I needed went away and my patches stopped
> compiling. Here is a new series.

Another update attached. The last CI run failed, but it seems to be a
problem with the cfbot. It had several green runs before that, and
everything still passes here. The error is:

Failed to start: INVALID_ARGUMENT: Operation with name
"operation-1761179023113-641c8720efc82-b98ffe61-7c88ff25" failed with
status = HttpJsonStatusCode{statusCode=PERMISSION_DENIED} and message
= FORBIDDEN

These new patches have some cleanup to the docs: whitespace, a bit of
clarification between application-time vs system-period PERIODs, and
removing the "periods are not supported" line in the final patch that
adds PERIODs.

The first 3 doc patches all apply to features that we released in v18,
so it would be nice to get those reviewed/merged soon if possible.

Patches 4-6 are another group, adding UPDATE/DELETE FOR PORTION OF.
That is the next step in SQL:2011 support. I think it is hard to use
temporal primary & foreign keys without temporal DML.

After that the patches are nice-to-have (especially foreign key
CASCADE), but less important IMO.

Also I apologize that those last attachments were out of order.
Hopefully it was user error so I can do something about it: I recently
switched from Thunderbird back to the Gmail web client. As I write
this email, Gmail is telling me the v57 files are in the right order,
so hopefully they stay that way after I send it.

Rebased to c0677d8b2e.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On 24.10.25 19:08, Paul A Jungwirth wrote:
> The first 3 doc patches all apply to features that we released in v18,
> so it would be nice to get those reviewed/merged soon if possible.

I have looked through the documentation patches 0001 through 0003.

I suggest making the Temporal Tables chapter a section instead.  It
doesn't feel big enough to be a top-level topic.  I think it would fit
well into the Data Definition chapter, perhaps after the "System
Columns" section (section 5.6).

And then the temporal update and delete material would go into the
Data Manipulation chapter.

The syntax examples for temporal primary keys would be better if they
used complete CREATE TABLE examples instead of ALTER TABLE on some
table that is presumed to exist.  (Or you could link to where in the
documentation the table is created.)

The PostgreSQL documentation is not really a place to describe
features that don't exist.  So while it's okay to mention system time
in the glossary because it contrasts with application time, it doesn't
seem appropriate to elaborate further on this in the main body of the
documentation, unless we actually implement it.  Similarly with
periods, we can document them when we have them, but before that it's
just a distraction.

The pictures are nice.  Again, it would be helpful if you showed the
full CREATE TABLE statement beforehand, so that it is easier to
picture when kind of table structure is being reflected.

Initially, I read $5, $8, etc. as parameter numbers, not as prices.
Perhaps possible confusion could be avoided if you notionally make the
price column of type numeric and show the prices like 5.00, 8.00, etc.

I also looked over the patch "Add UPDATE/DELETE FOR PORTION OF" a bit.
I think it has a good structure now.  I'll do a more detailed review
soon.

On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> On 24.10.25 19:08, Paul A Jungwirth wrote:
> > The first 3 doc patches all apply to features that we released in v18,
> > so it would be nice to get those reviewed/merged soon if possible.
>
> I have looked through the documentation patches 0001 through 0003.

Thanks for taking a look! New patches attached; details below.

Besides addressing your feedback, I corrected a few other details,
like a discrepancy in the valid-times between the SQL, the diagrams,
and the SELECT output.

> I suggest making the Temporal Tables chapter a section instead.  It
> doesn't feel big enough to be a top-level topic.  I think it would fit
> well into the Data Definition chapter, perhaps after the "System
> Columns" section (section 5.6).
>
> And then the temporal update and delete material would go into the
> Data Manipulation chapter.

Okay, done. This separation makes it a little awkward to continue the
example from the PKs/FKs section, but I included a link and repeated
the table contents, so I think it is okay. I agree it fits better into
the existing overall structure.

> The syntax examples for temporal primary keys would be better if they
> used complete CREATE TABLE examples instead of ALTER TABLE on some
> table that is presumed to exist.  (Or you could link to where in the
> documentation the table is created.)

I wound up creating the table without a PK first, then showing ALTER
TABLE to add the PK. I liked how this let me show temporal data in
general without addressing constraints right away.

> The PostgreSQL documentation is not really a place to describe
> features that don't exist.  So while it's okay to mention system time
> in the glossary because it contrasts with application time, it doesn't
> seem appropriate to elaborate further on this in the main body of the
> documentation, unless we actually implement it.  Similarly with
> periods, we can document them when we have them, but before that it's
> just a distraction.

Okay, I removed most of that. I left in a small note about not
supporting system time (not just in the glossary), because it is hard
to explain application time without the contrast. If you want me to
cut that too, please let me know.

The patch for documenting PERIODs is gone completely. I rolled that
into the main PERIODs patch. So now there are only two patches that
cover v18 functionality.

> The pictures are nice.  Again, it would be helpful if you showed the
> full CREATE TABLE statement beforehand, so that it is easier to
> picture when kind of table structure is being reflected.

I agree it is better that way.

> Initially, I read $5, $8, etc. as parameter numbers, not as prices.
> Perhaps possible confusion could be avoided if you notionally make the
> price column of type numeric and show the prices like 5.00, 8.00, etc.

Okay, changed to numeric and removed the dollar signs.

> I also looked over the patch "Add UPDATE/DELETE FOR PORTION OF" a bit.
> I think it has a good structure now.  I'll do a more detailed review
> soon.

Thanks!

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On Wed, Oct 29, 2025 at 11:02 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> > On 24.10.25 19:08, Paul A Jungwirth wrote:
> > > The first 3 doc patches all apply to features that we released in v18,
> > > so it would be nice to get those reviewed/merged soon if possible.
> >
> > I have looked through the documentation patches 0001 through 0003.
>
> Thanks for taking a look! New patches attached; details below.

Hi Hackers,

Here is another set of patches. I added isolation tests for FOR
PORTION OF. In REPEATABLE READ and SERIALIZABLE you get
easy-to-predict results. In READ COMMITTED you get a lot of lost
updates/deletes, because the second operation doesn't see the
leftovers created by the first (and sometimes the first operation
changes the start/end times in a way that EvalPlanQual no longer sees
the being-changed row either). I think those results make sense, if
you think step-by-step what Postgres is doing, but they are not really
what a user wants.

I tested the same sequences in MariaDB, and they also gave nonsense
results, although not always the same nonsense as Postgres. At
UNCOMMITTED READ it actually gave the results you'd want, but at that
level I assume you will have other problems.

I also tested DB2. It doesn't have READ COMMITTED, but I think READ
STABILITY is the closest. At that level (as well as CURSOR STABILITY
and REPEATABLE READ), you get correct results.

Back to Postgres, you can get "desired" results IN READ COMMITTED by
explicitly locking rows (with SELECT FOR UPDATE) just before
updating/deleting them. Since you acquire the lock before the
update/delete starts, there can be no new leftovers created within
that span of history, and the update/delete sees everything that is
there. The same approach also gives correct results in MariaDB. I
think it is just the way you have to do things with temporal tables in
READ COMMITTED whenever you expect concurrent updates to the same
history.

I considered whether we should make EvalPlanQual (or something else)
automatically rescan for leftovers when it's a temporal operation.
Then you wouldn't have to explicitly lock anything. But it seems like
that is more than the isolation level "contract", and maybe even plain
violates it (but arguably not, if you say the update shouldn't *start*
until the other session commits). But since there is a workaround, and
since other RDBMSes also scramble temporal data in READ COMMITTED, and
since it is a lot of work and seems tricky, I didn't attempt it.

Another idea (or maybe nearly the same thing) would be to
automatically do the same thing that SELECT FOR UPDATE is doing,
whenever we see a FOR PORTION OF DML command---i.e. scan for rows and
lock them first, then do the update. But that has similar issues. If
it adds locks the user doesn't expect, is it really the right thing?
And it means users pay the cost even when no concurrency is expected.
It offers strictly fewer options than requiring users to do SELECT FOR
UPDATE explicitly.

The isolation tests are a separate patch for now, because they felt
like a significant chunk, and I wanted to emphasize them, but really
they should be part of the main FOR PORTION OF commit. Probably I'll
squash them in future submissions. That patch also makes some small
updates to a comment in ExecForPortionOf and the docs for
UPDATE/DELETE FOR PORTION OF, to raise awareness of the READ COMMITTED
issues.

Rebased to 65f4976189.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On 30.10.25 07:02, Paul A Jungwirth wrote:
> On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> On 24.10.25 19:08, Paul A Jungwirth wrote:
>>> The first 3 doc patches all apply to features that we released in v18,
>>> so it would be nice to get those reviewed/merged soon if possible.
>>
>> I have looked through the documentation patches 0001 through 0003.
> 
> Thanks for taking a look! New patches attached; details below.
> 
> Besides addressing your feedback, I corrected a few other details,
> like a discrepancy in the valid-times between the SQL, the diagrams,
> and the SELECT output.
> 
>> I suggest making the Temporal Tables chapter a section instead.  It
>> doesn't feel big enough to be a top-level topic.  I think it would fit
>> well into the Data Definition chapter, perhaps after the "System
>> Columns" section (section 5.6).
>>
>> And then the temporal update and delete material would go into the
>> Data Manipulation chapter.
> 
> Okay, done. This separation makes it a little awkward to continue the
> example from the PKs/FKs section, but I included a link and repeated
> the table contents, so I think it is okay. I agree it fits better into
> the existing overall structure.

I committed the patches 0001 and 0002 (from v59).

I massaged it a bit to fit better into the flow of the chapter.  For 
example, there was already a "products" table mentioned earlier in the 
chapter, and I made the new one more similar to that one, so that it can 
be seen as an enhancement of what was already discussed.  Similarly, I 
changed the ALTER TABLE commands into CREATE TABLE, because in the 
chapter, the ALTER TABLE commands are not discussed until after the new 
section.  I also added some <emphasis> to the command examples, similar 
to what is done elsewhere.  There were some extra blank lines at the 
beginning of the image sources (.txt), which did show up as extra top 
padding in the SVG output, which didn't seem right.  I removed that and 
regenerated the images.  (Which worked well; I'm glad this pipeline 
still worked.)

On Tue, Nov 4, 2025 at 11:12 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
> Back to Postgres, you can get "desired" results IN READ COMMITTED by
> explicitly locking rows (with SELECT FOR UPDATE) just before
> updating/deleting them. Since you acquire the lock before the
> update/delete starts, there can be no new leftovers created within
> that span of history, and the update/delete sees everything that is
> there.

I forgot to mention: possibly we'll want to use this approach for
{CASCADE,SET {NULL,DEFAULT}} foreign keys (if the transaction is READ
COMMITTED). I'll explore that more and add it to the patch in this
series if it seems necessary. Also I didn't consider whether the
regular DML's lock could be weaker, like just KEY SHARE.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

I have looked at the patch

v59-0004-Add-range_minus_multi-and-multirange_minus_multi.patch

This seems sound in principle.

Perhaps you could restate why you chose a set-returning function rather 
than (what I suppose would be the other options) returning multirange or 
an array of ranges.  (I don't necessarily disagree, but it would be good 
to be clear for everyone.)  The point about allowing user-defined types 
makes sense (but for example, I see types like multipolygon and 
multipoint in postgis, so maybe those could also work?).

That said, I think there is a problem in your implementation.  Note that 
the added regression test cases for range return multiple rows but the 
ones for multirange all return a single row with a set {....} value.  I 
think the problem is that your multirange_minus_multi() calls 
multirange_minus_internal() which already returns a set, and you are 
packing that set result into a single row.

A few other minor details:

* src/backend/utils/adt/rangetypes.c

+#include "utils/array.h"

seems to be unused.

+   typedef struct
+   {
+       RangeType  *rs[2];
+       int         n;
+   }           range_minus_multi_fctx;

This could be written just as  a struct, like

struct range_minus_multi_fctx
{
...
};

Wrapping it in a typedef doesn't achieve any additional useful
abstraction.

The code comment before range_minus_multi_internal() could first
explain briefly what the function does before going into the details
of the arguments.  Because we can't assume that someone will have read
the descriptions of the higher-level functions first.

* src/include/catalog/pg_proc.dat

The prorows values for the two new functions should be the same?

(I suppose they are correct now seeing your implementation of 
multirange_minus_multi(), but I'm not sure that was intended, as 
discussed above.)

> On Nov 5, 2025, at 03:12, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
>
> On Wed, Oct 29, 2025 at 11:02 PM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
>>
>> On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>>> On 24.10.25 19:08, Paul A Jungwirth wrote:
>>>> The first 3 doc patches all apply to features that we released in v18,
>>>> so it would be nice to get those reviewed/merged soon if possible.
>>>
>>> I have looked through the documentation patches 0001 through 0003.
>>
>> Thanks for taking a look! New patches attached; details below.
>
> Hi Hackers,
>
> Here is another set of patches. I added isolation tests for FOR
> PORTION OF. In REPEATABLE READ and SERIALIZABLE you get
> easy-to-predict results. In READ COMMITTED you get a lot of lost
> updates/deletes, because the second operation doesn't see the
> leftovers created by the first (and sometimes the first operation
> changes the start/end times in a way that EvalPlanQual no longer sees
> the being-changed row either). I think those results make sense, if
> you think step-by-step what Postgres is doing, but they are not really
> what a user wants.
>
> I tested the same sequences in MariaDB, and they also gave nonsense
> results, although not always the same nonsense as Postgres. At
> UNCOMMITTED READ it actually gave the results you'd want, but at that
> level I assume you will have other problems.
>
> I also tested DB2. It doesn't have READ COMMITTED, but I think READ
> STABILITY is the closest. At that level (as well as CURSOR STABILITY
> and REPEATABLE READ), you get correct results.
>
> Back to Postgres, you can get "desired" results IN READ COMMITTED by
> explicitly locking rows (with SELECT FOR UPDATE) just before
> updating/deleting them. Since you acquire the lock before the
> update/delete starts, there can be no new leftovers created within
> that span of history, and the update/delete sees everything that is
> there. The same approach also gives correct results in MariaDB. I
> think it is just the way you have to do things with temporal tables in
> READ COMMITTED whenever you expect concurrent updates to the same
> history.
>
> I considered whether we should make EvalPlanQual (or something else)
> automatically rescan for leftovers when it's a temporal operation.
> Then you wouldn't have to explicitly lock anything. But it seems like
> that is more than the isolation level "contract", and maybe even plain
> violates it (but arguably not, if you say the update shouldn't *start*
> until the other session commits). But since there is a workaround, and
> since other RDBMSes also scramble temporal data in READ COMMITTED, and
> since it is a lot of work and seems tricky, I didn't attempt it.
>
> Another idea (or maybe nearly the same thing) would be to
> automatically do the same thing that SELECT FOR UPDATE is doing,
> whenever we see a FOR PORTION OF DML command---i.e. scan for rows and
> lock them first, then do the update. But that has similar issues. If
> it adds locks the user doesn't expect, is it really the right thing?
> And it means users pay the cost even when no concurrency is expected.
> It offers strictly fewer options than requiring users to do SELECT FOR
> UPDATE explicitly.
>
> The isolation tests are a separate patch for now, because they felt
> like a significant chunk, and I wanted to emphasize them, but really
> they should be part of the main FOR PORTION OF commit. Probably I'll
> squash them in future submissions. That patch also makes some small
> updates to a comment in ExecForPortionOf and the docs for
> UPDATE/DELETE FOR PORTION OF, to raise awareness of the READ COMMITTED
> issues.
>
> Rebased to 65f4976189.
>
> Yours,
>
> --
> Paul              ~{:-)
> pj@illuminatedcomputing.com
>
<v59-0003-Document-temporal-update-delete.patch><v59-0005-Add-UPDATE-DELETE-FOR-PORTION-OF.patch><v59-0001-Add-docs-section-for-temporal-tables-with-primar.patch><v59-0004-Add-range_minus_multi-and-multirange_minus_multi.patch><v59-0007-Add-tg_temporal-to-TriggerData.patch><v59-0002-Document-temporal-foreign-keys.patch><v59-0008-Look-up-more-temporal-foreign-key-helper-procs.patch><v59-0009-Add-CASCADE-SET-NULL-SET-DEFAULT-for-temporal-fo.patch><v59-0006-Add-isolation-tests-for-UPDATE-DELETE-FOR-PORTIO.patch><v59-0011-Add-PERIODs.patch><v59-0010-Expose-FOR-PORTION-OF-to-plpgsql-triggers.patch>

I tried to review this patch. Though I “git reset” to commit 65f4976189, “git am” still failed at 0009.

Today I only reviewed 0001, it was a happy reading. I found a small typo and got a suggestion:

1 - 0001
```
+    entity described by a table. In a typical non-temporal table, there is
+    single row for each entity. In a temporal table, an entity may have
```

“There is single row” should be “there is a single row”.


2 - 0001 - The doc mentions rangetypes which is the key factor for defining a temporal table, can we add a hyper link
on“rangetype” so that readers can easily jump to learn which rangetypes can be used. 

I will continue to review the rest of commits tomorrow.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

> On Nov 12, 2025, at 17:31, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
>> On Nov 5, 2025, at 03:12, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
>>
>> On Wed, Oct 29, 2025 at 11:02 PM Paul A Jungwirth
>> <pj@illuminatedcomputing.com> wrote:
>>>
>>> On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>>>> On 24.10.25 19:08, Paul A Jungwirth wrote:
>>>>> The first 3 doc patches all apply to features that we released in v18,
>>>>> so it would be nice to get those reviewed/merged soon if possible.
>>>>
>>>> I have looked through the documentation patches 0001 through 0003.
>>>
>>> Thanks for taking a look! New patches attached; details below.
>>
>> Hi Hackers,
>>
>> Here is another set of patches. I added isolation tests for FOR
>> PORTION OF. In REPEATABLE READ and SERIALIZABLE you get
>> easy-to-predict results. In READ COMMITTED you get a lot of lost
>> updates/deletes, because the second operation doesn't see the
>> leftovers created by the first (and sometimes the first operation
>> changes the start/end times in a way that EvalPlanQual no longer sees
>> the being-changed row either). I think those results make sense, if
>> you think step-by-step what Postgres is doing, but they are not really
>> what a user wants.
>>
>> I tested the same sequences in MariaDB, and they also gave nonsense
>> results, although not always the same nonsense as Postgres. At
>> UNCOMMITTED READ it actually gave the results you'd want, but at that
>> level I assume you will have other problems.
>>
>> I also tested DB2. It doesn't have READ COMMITTED, but I think READ
>> STABILITY is the closest. At that level (as well as CURSOR STABILITY
>> and REPEATABLE READ), you get correct results.
>>
>> Back to Postgres, you can get "desired" results IN READ COMMITTED by
>> explicitly locking rows (with SELECT FOR UPDATE) just before
>> updating/deleting them. Since you acquire the lock before the
>> update/delete starts, there can be no new leftovers created within
>> that span of history, and the update/delete sees everything that is
>> there. The same approach also gives correct results in MariaDB. I
>> think it is just the way you have to do things with temporal tables in
>> READ COMMITTED whenever you expect concurrent updates to the same
>> history.
>>
>> I considered whether we should make EvalPlanQual (or something else)
>> automatically rescan for leftovers when it's a temporal operation.
>> Then you wouldn't have to explicitly lock anything. But it seems like
>> that is more than the isolation level "contract", and maybe even plain
>> violates it (but arguably not, if you say the update shouldn't *start*
>> until the other session commits). But since there is a workaround, and
>> since other RDBMSes also scramble temporal data in READ COMMITTED, and
>> since it is a lot of work and seems tricky, I didn't attempt it.
>>
>> Another idea (or maybe nearly the same thing) would be to
>> automatically do the same thing that SELECT FOR UPDATE is doing,
>> whenever we see a FOR PORTION OF DML command---i.e. scan for rows and
>> lock them first, then do the update. But that has similar issues. If
>> it adds locks the user doesn't expect, is it really the right thing?
>> And it means users pay the cost even when no concurrency is expected.
>> It offers strictly fewer options than requiring users to do SELECT FOR
>> UPDATE explicitly.
>>
>> The isolation tests are a separate patch for now, because they felt
>> like a significant chunk, and I wanted to emphasize them, but really
>> they should be part of the main FOR PORTION OF commit. Probably I'll
>> squash them in future submissions. That patch also makes some small
>> updates to a comment in ExecForPortionOf and the docs for
>> UPDATE/DELETE FOR PORTION OF, to raise awareness of the READ COMMITTED
>> issues.
>>
>> Rebased to 65f4976189.
>>
>> Yours,
>>
>> --
>> Paul              ~{:-)
>> pj@illuminatedcomputing.com
>>
<v59-0003-Document-temporal-update-delete.patch><v59-0005-Add-UPDATE-DELETE-FOR-PORTION-OF.patch><v59-0001-Add-docs-section-for-temporal-tables-with-primar.patch><v59-0004-Add-range_minus_multi-and-multirange_minus_multi.patch><v59-0007-Add-tg_temporal-to-TriggerData.patch><v59-0002-Document-temporal-foreign-keys.patch><v59-0008-Look-up-more-temporal-foreign-key-helper-procs.patch><v59-0009-Add-CASCADE-SET-NULL-SET-DEFAULT-for-temporal-fo.patch><v59-0006-Add-isolation-tests-for-UPDATE-DELETE-FOR-PORTIO.patch><v59-0011-Add-PERIODs.patch><v59-0010-Expose-FOR-PORTION-OF-to-plpgsql-triggers.patch>
>
> I tried to review this patch. Though I “git reset” to commit 65f4976189, “git am” still failed at 0009.
>
> Today I only reviewed 0001, it was a happy reading. I found a small typo and got a suggestion:
>
> 1 - 0001
> ```
> +    entity described by a table. In a typical non-temporal table, there is
> +    single row for each entity. In a temporal table, an entity may have
> ```
>
> “There is single row” should be “there is a single row”.
>
>
> 2 - 0001 - The doc mentions rangetypes which is the key factor for defining a temporal table, can we add a hyper link
on“rangetype” so that readers can easily jump to learn which rangetypes can be used. 
>
> I will continue to review the rest of commits tomorrow.
>

I spent a hour reading through 0002-0004 and got my brain stuck. I’d stop here today, and maybe continue tomorrow.

A few more comments:

3 - 0002
```
+<programlisting>
+CREATE TABLE variants (
+  id         integer   NOT NULL,
+  product_id integer   NOT NULL,
+  name       text      NOT NULL,
+  valid_at   daterange NOT NULL,
+  CONSTRAINT variants_pkey
+    PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
+);
+</programlisting>
```

The common before ) is not needed.

4 - 0002
```
+    <para>
+
+     In a table, these records would be:
+<programlisting>
+ id | product_id |  name  |        valid_at
+----+------------+--------+-------------------------
+  8 |          5 | Medium | [2021-01-01,2023-06-01)
+  9 |          5 | XXL    | [2022-03-01,2024-06-01)
+</programlisting>
+    </para>
```

The blank line after “<para>” is not needed.

5 - 0003
```
+     zero, one, or two stretches of history that where not updated/deleted
```

Typo: where -> were

6 - 0004 - func-range.sgml
```
      <row>
       <entry role="func_table_entry"><para role="func_signature">
        <indexterm>
         <primary>multirange_minus_multi</primary>
        </indexterm>
        <function>multirange_minus_multi</function> ( <type>anymultirange</type>, <type>anymultirange</type> )
        <returnvalue>setof anymultirange</returnvalue>
       </para>
       <para>
        Returns the non-empty multirange(s) remaining after subtracting the second multirange from the first.
        If the subtraction yields an empty multirange, no rows are returned.
        Two rows are never returned, because a single multirange can always accommodate any result.
       </para>
       <para>
        <literal>range_minus_multi('[0,10)'::int4range, '[3,4)'::int4range)</literal>
        <returnvalue>{[0,3), [4,10)}</returnvalue>
       </para></entry>
      </row>
```

I believe in " <literal>range_minus_multi('[0,10)'::int4range, '[3,4)'::int4range)</literal>”, it should be
“multirange_minus_multi”.


Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Tue, Nov 11, 2025 at 11:42 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I have looked at the patch
>
> v59-0004-Add-range_minus_multi-and-multirange_minus_multi.patch
>
> This seems sound in principle.

Thank you for the review! I've attached new patches addressing the
feedback from you and Chao Li. Details below:

> Perhaps you could restate why you chose a set-returning function rather
> than (what I suppose would be the other options) returning multirange or
> an array of ranges.  (I don't necessarily disagree, but it would be good
> to be clear for everyone.)  The point about allowing user-defined types
> makes sense (but for example, I see types like multipolygon and
> multipoint in postgis, so maybe those could also work?).

Allowing user-defined types is the main motivation. I wanted
ExecForPortionOfLeftovers to avoid type-specific logic, so that users
could use whatever type they like. As you say, spatial types seem like
a natural fit. I'm also interested in using FOR PORTION OF with a
future extension for mdranges ("multi-dimensional ranges"), which
would let people track multiple dimensions of application time. At
least one author (Tom Johnston) refers to this as "assertion time",
where a dimension represents a truth claim about the world. Others
have also expressed interest in "tri-temporal" tables. I think people
could come up with all kinds of interesting ways to use this feature.

So we need a function that takes the existing row's value (in some
type T) and subtracts the value targeted by the update/delete. It
needs to return zero or more Ts, one for each temporal leftover. It
can't return an array of Ts, because anyrange doesn't work that way.
(Likewise anymultirange.) Given a function with an anyrange argument
and an anyarray return value, Postgres expects an array of the range's
*base type*. In other words we can do this:

array<T> minus_multi<T>(range<T> r1, range<T> r2)

but not this:

array<T> minus_multi<T where T is rangetype>(T r1, T r2)

But what I want *is* possible as a set-returning function. Because
then the signature is just `anyrange f(anyrange, anyrange)`.

> That said, I think there is a problem in your implementation.  Note that
> the added regression test cases for range return multiple rows but the
> ones for multirange all return a single row with a set {....} value.  I
> think the problem is that your multirange_minus_multi() calls
> multirange_minus_internal() which already returns a set, and you are
> packing that set result into a single row.

I think you are misunderstanding. The curly braces are just the
multirange string notation, not a set. (Mathematically a multirange is
a set though.) The function is still a Set-Returning Function, to
match the interface we want, but it never needs to return more than
one row, because a single multirange can always accommodate the result
of mr1 - mr2 (unlike with range types). Note it can *also* return zero
rows, if the result would be empty. (There are examples of this in the
regress tests.) Each row from these SRFs becomes an INSERTed temporal
leftover in ExecForPortionOfLeftovers. Multiranges can insert zero or
one. Ranges can insert up to two. A user-defined type might insert
more.

> A few other minor details:
>
> * src/backend/utils/adt/rangetypes.c
>
> +#include "utils/array.h"
>
> seems to be unused.

You're right; removed.

> +   typedef struct
> +   {
> +       RangeType  *rs[2];
> +       int         n;
> +   }           range_minus_multi_fctx;
>
> This could be written just as  a struct, like
>
> struct range_minus_multi_fctx
> {
> ...
> };
>
> Wrapping it in a typedef doesn't achieve any additional useful
> abstraction.

Okay.

> The code comment before range_minus_multi_internal() could first
> explain briefly what the function does before going into the details
> of the arguments.  Because we can't assume that someone will have read
> the descriptions of the higher-level functions first.

Done, with some extra word-smithing.

> * src/include/catalog/pg_proc.dat
>
> The prorows values for the two new functions should be the same?
>
> (I suppose they are correct now seeing your implementation of
> multirange_minus_multi(), but I'm not sure that was intended, as
> discussed above.)

Right, rangetypes are prorows 2 and multiranges are prorows 1.

I'll reply to Chao Li separately, but those changes are included in
the patches here.

Rebased to 705601c5ae.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

> On Nov 13, 2025, at 12:07, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
>
> I'll reply to Chao Li separately, but those changes are included in
> the patches here.
>
> Rebased to 705601c5ae.
>
> Yours,
>
> --
> Paul              ~{:-)
> pj@illuminatedcomputing.com
>
<v60-0001-Fix-typo-in-documentation-about-application-time.patch><v60-0004-Add-UPDATE-DELETE-FOR-PORTION-OF.patch><v60-0003-Add-range_minus_multi-and-multirange_minus_multi.patch><v60-0002-Document-temporal-update-delete.patch><v60-0005-Add-isolation-tests-for-UPDATE-DELETE-FOR-PORTIO.patch><v60-0006-Add-tg_temporal-to-TriggerData.patch><v60-0009-Expose-FOR-PORTION-OF-to-plpgsql-triggers.patch><v60-0007-Look-up-more-temporal-foreign-key-helper-procs.patch><v60-0008-Add-CASCADE-SET-NULL-SET-DEFAULT-for-temporal-fo.patch><v60-0010-Add-PERIODs.patch>


I continue reviewing ...

Even if I have hard reset to 705601c5ae, “git am” still failed at 0009. Anyway, I guess I cannot reach that far today.

0001, 0002 (was 0003) and 0003 (was 0004) have addressed my previous comments, now looks good to me.

I will number the comments continuously.

7 - 0004 - create_publication.sgml
```
+   For a <command>FOR PORTION OF</command> command, the publication will publish an
```

This is a little confusing, “FOR PORTION OF” is not a command, it’s just a clause inside UDDATE or DELETE. So maybe
changeto: 

For an <command>UPDATE/DELETE ... FOR PORTION OF<command> clause …

8 - 0004 - delete.sgml
```
+   you may supply a <literal>FOR PORTION OF</literal> clause, and your delete will
+   only affect rows that overlap the given interval. Furthermore, if a row's history
+   extends outside the <literal>FOR PORTION OF</literal> bounds, then your delete
```

“Your delete” sounds not formal doc style. I searched over all docs and didn’t found other occurrence.

9 - 0004 - update.sgml
```
+   you may supply a <literal>FOR PORTION OF</literal> clause, and your update will
+   only affect rows that overlap the given interval. Furthermore, if a row's history
+   extends outside the <literal>FOR PORTION OF</literal> bounds, then your update
```

“Your update”, same comment as 8.

10 - 0004 - update.sgml
```
+   Specifically, when <productname>PostgreSQL</productname> updates the existing row,
+   it will also change the range or multirange so that their interval
```

“Update the existing row”, here I think “an” is better than “the”, because we are not referring to any specific row.
Then, “there interval” should be “its interval”.

11 - 0004 - update.sgml
```
+   the targeted bounds, with un-updated values in their other columns.
```

“Un-updated” sounds strange, I never saw that. Maybe “unchanged”?

12 - 0004 - update.sgml
```
+   There will be zero to two inserted records,
```

I don’t fully get this. Say, original range is 2-5:

* if update 1-6, then no insert;
* if update 3-4, then two inserts
* if update 2-4, should it be just one insert?

13 - 0004 - nodeModifyTable.c
```
+    /*
+     * Get the old pre-UPDATE/DELETE tuple. We will use its range to compute
+     * untouched parts of history, and if necessary we will insert copies
+     * with truncated start/end times.
+     *
+     * We have already locked the tuple in ExecUpdate/ExecDelete, and it has
+     * passed EvalPlanQual. This ensures that concurrent updates in READ
+     * COMMITTED can't insert conflicting temporal leftovers.
+     */
+    if (!table_tuple_fetch_row_version(resultRelInfo->ri_RelationDesc, tupleid, SnapshotAny, oldtupleSlot))
+        elog(ERROR, "failed to fetch tuple for FOR PORTION OF”);
```

I have a question and don’t find the answer from the code change.

For update, the old row will point to the newly inserted row, so that there is chain of history rows. With portion
update,from an old row it has no way to find the newly inserted row, is this a concern? 

14 - 0004 - nodeModifyTable.c
```
+            elog(ERROR, "Got a null from without_portion function”);
```

Nit: it’s unusual to start elog with a capital letter, so “Got” -> “got”.

15 - 0004 - nodeModifyTable.c
```
+        if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+            mtstate->mt_partition_tuple_routing == NULL)
+        {
+            /*
+             * We will need tuple routing to insert temporal leftovers. Since
+             * we are initializing things before ExecCrossPartitionUpdate
+             * runs, we must do everything it needs as well.
+             */
+            if (mtstate->mt_partition_tuple_routing == NULL)
+            {
```

The outer “if” has checked mtstate->mt_partition_tuple_routing == NULL, so the inner “if” is a redundant.

16 - 0004 - nodeFuncs.c
```
+        case T_ForPortionOfExpr:
+            {
+                ForPortionOfExpr *forPortionOf = (ForPortionOfExpr *) node;
+
+                if (WALK(forPortionOf->targetRange))
+                    return true;
+            }
+            break;
```

I am not sure, but do we also need to walk rangeVar and rangeTargetList?

17 - 0004 - analyze.c
```
+static Node *
+addForPortionOfWhereConditions(Query *qry, ForPortionOfClause *forPortionOf, Node *whereClause)
+{
+    if (forPortionOf)
+    {
+        if (whereClause)
+            return (Node *) makeBoolExpr(AND_EXPR, list_make2(qry->forPortionOf->overlapsExpr, whereClause), -1);
+        else
+            return qry->forPortionOf->overlapsExpr;
```

Do we need to check if qry->forPortionOf is NULL?

Wow, 0004 is too long, I’d stop here today, continue with the rest tomorrow.

18 - 0005 - dml.sgml
```
+   In <literal>READ COMMITTED</literal> mode, temporal updates and deletes can
+   cause unexpected results when they concurrently touch the same row. It is
```

“Cause unexpected results” sounds not formal doc style, suggesting “may yield results that differ from what the user
intends”.

19 - 0006 - tablecmds.c
```
@@ -13760,6 +13760,7 @@ validateForeignKeyConstraint(char *conname,
         trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, false, NULL);
         trigdata.tg_trigslot = slot;
         trigdata.tg_trigger = &trig;
+        trigdata.tg_temporal = NULL;
```

Looks like no need to assign NULL to trigdata.tg_temporal, because “trigdata” has bee zero-ed when defining it. In
otherplaces of this patch, you don’t additionally initialize it, so this place might not need as well. 

20 - 0007 - pg_constraint.c
```
 void
-FindFKPeriodOpers(Oid opclass,
-                  Oid *containedbyoperoid,
-                  Oid *aggedcontainedbyoperoid,
-                  Oid *intersectoperoid)
+FindFKPeriodOpersAndProcs(Oid opclass,
+                          Oid *containedbyoperoid,
+                          Oid *aggedcontainedbyoperoid,
+                          Oid *intersectoperoid,
+                          Oid *intersectprocoid,
+                          Oid *withoutportionoid)
 {
     Oid            opfamily = InvalidOid;
     Oid            opcintype = InvalidOid;
@@ -1693,6 +1700,17 @@ FindFKPeriodOpers(Oid opclass,
                                aggedcontainedbyoperoid,
                                &strat);

+    /*
+     * Hardcode intersect operators for ranges and multiranges, because we
+     * don't have a better way to look up operators that aren't used in
+     * indexes.
+     *
+     * If you change this code, you must change the code in
+     * transformForPortionOfClause.
+     *
+     * XXX: Find a more extensible way to look up the operator, permitting
+     * user-defined types.
+     */
     switch (opcintype)
     {
         case ANYRANGEOID:
@@ -1704,6 +1722,14 @@ FindFKPeriodOpers(Oid opclass,
         default:
             elog(ERROR, "unexpected opcintype: %u", opcintype);
     }
+
+    /*
+     * Look up the intersect proc. We use this for FOR PORTION OF (both the
+     * operation itself and when checking foreign keys). If this is missing we
+     * don't need to complain here, because FOR PORTION OF will not be
+     * allowed.
+     */
+    *intersectprocoid = get_opcode(*intersectoperoid);
 }
```

I don’t see withoutportionoid is initialized.

21 - 0008 - ri_triggers.c
```
+            quoteOneName(attname,
+                         RIAttName(fk_rel, riinfo->fk_attnums[i]));
```

This patch uses quoteOneName() a lot. This function simply add double quotes without much checks which is unsafe. I
thinkquote_identifier() is more preferred. 

22 - 0009 - pl_exec.c
```
+        case PLPGSQL_PROMISE_TG_PERIOD_BOUNDS:
+            fpo = estate->trigdata->tg_temporal;
+
+            if (estate->trigdata == NULL)
+                elog(ERROR, "trigger promise is not in a trigger function");
```

You deference estate->trigdata before the NULL check. So the “fpo” assignment should be moved to after the NULL check.

23 - 0009 - pl_comp.c
```
+            /*
+             * Add the variable to tg_period_bounds. This could be any
```

Nit typo: “to” is not needed.

Wow, 0010 is too big, I have spent the entire morning, so I’d leave 0010 to next week.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

> On Nov 14, 2025, at 12:10, Chao Li <li.evan.chao@gmail.com> wrote:
>
> 21 - 0008 - ri_triggers.c
> ```
> + quoteOneName(attname,
> +  RIAttName(fk_rel, riinfo->fk_attnums[i]));
> ```
>
> This patch uses quoteOneName() a lot. This function simply add double quotes without much checks which is unsafe. I
thinkquote_identifier() is more preferred. 

I looked further, and realized that quoteOneName() is widely used in ri_triggers.c and the dest string are all defined
assize of MAX_QUOTED_REL_NAME_LEN. 

So I take back comment 21.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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.

> Even if I have hard reset to 705601c5ae, “git am” still failed at 0009. Anyway, I guess I cannot reach that far
today.

I tested them out against 705601c5ae with `git am v60*` and got a
couple whitespace warnings, but otherwise they applied. Those warnings
are fixed in this batch, and the v61 patches apply against master for
me. If you still have problems, can you share the command you're using
and its output?

> 7 - 0004 - create_publication.sgml
> ```
> +   For a <command>FOR PORTION OF</command> command, the publication will publish an
> ```
>
> This is a little confusing, “FOR PORTION OF” is not a command, it’s just a clause inside UDDATE or DELETE. So maybe
changeto: 
>
> For an <command>UPDATE/DELETE ... FOR PORTION OF<command> clause …

Okay.

> 8 - 0004 - delete.sgml
> ```
> +   you may supply a <literal>FOR PORTION OF</literal> clause, and your delete will
> +   only affect rows that overlap the given interval. Furthermore, if a row's history
> +   extends outside the <literal>FOR PORTION OF</literal> bounds, then your delete
> ```
>
> “Your delete” sounds not formal doc style. I searched over all docs and didn’t found other occurrence.

Okay.

> 9 - 0004 - update.sgml
> ```
> +   you may supply a <literal>FOR PORTION OF</literal> clause, and your update will
> +   only affect rows that overlap the given interval. Furthermore, if a row's history
> +   extends outside the <literal>FOR PORTION OF</literal> bounds, then your update
> ```
>
> “Your update”, same comment as 8.

Okay.

> 10 - 0004 - update.sgml
> ```
> +   Specifically, when <productname>PostgreSQL</productname> updates the existing row,
> +   it will also change the range or multirange so that their interval
> ```
>
> “Update the existing row”, here I think “an” is better than “the”, because we are not referring to any specific row.
> Then, “there interval” should be “its interval”.

Okay.

> 11 - 0004 - update.sgml
> ```
> +   the targeted bounds, with un-updated values in their other columns.
> ```
>
> “Un-updated” sounds strange, I never saw that. Maybe “unchanged”?

Changed to "the original values".

> 12 - 0004 - update.sgml
> ```
> +   There will be zero to two inserted records,
> ```
>
> I don’t fully get this. Say, original range is 2-5:
>
> * if update 1-6, then no insert;
> * if update 3-4, then two inserts
> * if update 2-4, should it be just one insert?

I agree an example is nice. I reworked this a bit.

> 13 - 0004 - nodeModifyTable.c
> ```
> +       /*
> +        * Get the old pre-UPDATE/DELETE tuple. We will use its range to compute
> +        * untouched parts of history, and if necessary we will insert copies
> +        * with truncated start/end times.
> +        *
> +        * We have already locked the tuple in ExecUpdate/ExecDelete, and it has
> +        * passed EvalPlanQual. This ensures that concurrent updates in READ
> +        * COMMITTED can't insert conflicting temporal leftovers.
> +        */
> +       if (!table_tuple_fetch_row_version(resultRelInfo->ri_RelationDesc, tupleid, SnapshotAny, oldtupleSlot))
> +               elog(ERROR, "failed to fetch tuple for FOR PORTION OF”);
> ```
>
> I have a question and don’t find the answer from the code change.
>
> For update, the old row will point to the newly inserted row, so that there is chain of history rows. With portion
update,from an old row it has no way to find the newly inserted row, is this a concern? 

True, there is not a connection from the newly-inserted rows to the
old updated row (other than the scalar part(s) of the primary key). I
think that is correct as far as lower-level details go. It might be
nice to have something for triggers though, similar to how I'm
exposing the TO/FROM bounds, and then users could set a column if they
like. The standard doesn't suggest anything like that, but we could
add it. I think it can be a separate follow-on patch though.

> 14 - 0004 - nodeModifyTable.c
> ```
> +                       elog(ERROR, "Got a null from without_portion function”);
> ```
>
> Nit: it’s unusual to start elog with a capital letter, so “Got” -> “got”.

Okay.

> 15 - 0004 - nodeModifyTable.c
> ```
> +               if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
> +                       mtstate->mt_partition_tuple_routing == NULL)
> +               {
> +                       /*
> +                        * We will need tuple routing to insert temporal leftovers. Since
> +                        * we are initializing things before ExecCrossPartitionUpdate
> +                        * runs, we must do everything it needs as well.
> +                        */
> +                       if (mtstate->mt_partition_tuple_routing == NULL)
> +                       {
> ```
>
> The outer “if” has checked mtstate->mt_partition_tuple_routing == NULL, so the inner “if” is a redundant.

You're right, fixed.

> 16 - 0004 - nodeFuncs.c
> ```
> +               case T_ForPortionOfExpr:
> +                       {
> +                               ForPortionOfExpr *forPortionOf = (ForPortionOfExpr *) node;
> +
> +                               if (WALK(forPortionOf->targetRange))
> +                                       return true;
> +                       }
> +                       break;
> ```
>
> I am not sure, but do we also need to walk rangeVar and rangeTargetList?

No. Postgres builds both of those during analysis from simple Var nodes.

> 17 - 0004 - analyze.c
> ```
> +static Node *
> +addForPortionOfWhereConditions(Query *qry, ForPortionOfClause *forPortionOf, Node *whereClause)
> +{
> +       if (forPortionOf)
> +       {
> +               if (whereClause)
> +                       return (Node *) makeBoolExpr(AND_EXPR, list_make2(qry->forPortionOf->overlapsExpr,
whereClause),-1); 
> +               else
> +                       return qry->forPortionOf->overlapsExpr;
> ```
>
> Do we need to check if qry->forPortionOf is NULL?

It should be set if forPortionOf is set. I added an Assert for it.

> 18 - 0005 - dml.sgml
> ```
> +   In <literal>READ COMMITTED</literal> mode, temporal updates and deletes can
> +   cause unexpected results when they concurrently touch the same row. It is
> ```
>
> “Cause unexpected results” sounds not formal doc style, suggesting “may yield results that differ from what the user
intends”.

That seems quite verbose. I found many examples of "unexpected
results". I changed "change" to "yield" though, which matches existing
documentation.

> 19 - 0006 - tablecmds.c
> ```
> @@ -13760,6 +13760,7 @@ validateForeignKeyConstraint(char *conname,
>                 trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, false, NULL);
>                 trigdata.tg_trigslot = slot;
>                 trigdata.tg_trigger = &trig;
> +               trigdata.tg_temporal = NULL;
> ```
>
> Looks like no need to assign NULL to trigdata.tg_temporal, because “trigdata” has bee zero-ed when defining it. In
otherplaces of this patch, you don’t additionally initialize it, so this place might not need as well. 

Okay.

> 20 - 0007 - pg_constraint.c
> ...
> I don’t see withoutportionoid is initialized.

You're right, this is not actually used by foreign keys anymore. It
was required for RESTRICT, but we decided to leave that out for now,
and I thought at first I would also need it for CASCADE/SET NULL/SET
DEFAULT, but then I realized those operations didn't require it. It
looks like I only partially removed it though.

> 21 - 0008 - ri_triggers.c
> ```
> +                       quoteOneName(attname,
> +                                                RIAttName(fk_rel, riinfo->fk_attnums[i]));
> ```
>
> This patch uses quoteOneName() a lot. This function simply add double quotes without much checks which is unsafe. I
thinkquote_identifier() is more preferred. 

As you say in your followup, quoteOneName is used extensively in the
foreign key code to quote columns. It's defined in ri_triggers.c. I
don't think it is unsafe here. We should follow what the surrounding
code is doing.

> 22 - 0009 - pl_exec.c
> ```
> +               case PLPGSQL_PROMISE_TG_PERIOD_BOUNDS:
> +                       fpo = estate->trigdata->tg_temporal;
> +
> +                       if (estate->trigdata == NULL)
> +                               elog(ERROR, "trigger promise is not in a trigger function");
> ```
>
> You deference estate->trigdata before the NULL check. So the “fpo” assignment should be moved to after the NULL
check.

You're right! Fixed.

> 23 - 0009 - pl_comp.c
> ```
> +                       /*
> +                        * Add the variable to tg_period_bounds. This could be any
> ```
>
> Nit typo: “to” is not needed.

Okay.

Rebased to d5b4f3a6d4.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

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.

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.

--
Paul              ~{:-)
pj@illuminatedcomputing.com

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.

On Thu, Nov 27, 2025 at 7:44 AM Peter Eisentraut <peter@eisentraut.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@illuminatedcomputing.com

On Fri, Dec 5, 2025 at 4:42 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Thu, Nov 27, 2025 at 7:44 AM Peter Eisentraut <peter@eisentraut.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.

Rebased to fix some conflicts. I'm leaving out the final PERIODs patch
in this set. Maybe I will continue skipping it since it is frequently
the cause of rebase conflicts. And I think of it as a "next step"
after this other work is finished.

I don't think there's much else new here, except I expanded the main
patch's commit message a bit.

Rebased to d49936f3028.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com