Обсуждение: 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/