Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Make ExecForPortionOfLeftovers() obey SRF protocol.
- 207cb2abcba0 19 (unreleased) landed
Add isolation tests for UPDATE/DELETE FOR PORTION OF
- b6ccd30d8ff6 19 (unreleased) landed
Add UPDATE/DELETE FOR PORTION OF
- 8e72d914c528 19 (unreleased) landed
Record range constructor functions in pg_range
- c257ba839718 19 (unreleased) landed
Add range_minus_multi and multirange_minus_multi functions
- 5eed8ce50ce9 19 (unreleased) landed
doc: Add section for temporal tables
- e4d8a2af07f5 19 (unreleased) landed
Add assertion check for WAL receiver state during stream-archive transition
- 65f4976189b6 19 (unreleased) cited

SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-06-02T05:24:44Z

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-06-22T23:19:20Z

On Sun, Jun 1, 2025 at 10:24 PM Paul Jungwirth
<pj@illuminatedcomputing.com> wrote:
> Please stay tuned for some actual patches!

Hi Hackers,

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. (Every time cataloged NOT
NULL constraints change, it has rebase conflicts. :-)

I wrote a long wiki page to summarize progress on this patch and other
application-time patches:
https://wiki.postgresql.org/wiki/ApplicationTimeProgress The main goal
is to record design decisions and their rationale, so we don't have to
revisit those or scour the archives for them. It also has a "Progress"
section to show what is done and what remains. Hopefully that will
help people jump in and understand what's happening. I'll link to it
from the commitfest entry and keep it up-to-date.

That page does *not* introduce general concepts for application time,
although I think that is needed too. But we already have another page
for that (sort of). I added an application-time section to this old
wiki page: https://wiki.postgresql.org/wiki/SQL2011Temporal Before my
edits, that page only covered System Time, along with a proposal from
2012-2015 for implementing it with triggers. I kept all that but moved
it into a "System Time" section.

Notable things about the current patch set:

- I added a new chapter to the docs to introduce temporal concepts.
This gives us a more convenient place to explain concepts and link to
them. I made separate patches for primary keys and foreign keys, in
case we want to include those in v18. I made a separate patch for
PERIODs also, which we could include now if we wanted: it explains
that the current functionality uses ranges & multiranges, but we plan
to support periods in the future. The last doc patch is for
UPDATE/DELETE FOR PORTION OF. It introduces the term "temporal
leftovers", which is very helpful when explaining the implicit INSERTs
from an UPDATE/DELETE FOR PORTION OF. Those patches add some more
glossary entries as well. I also tried to improve how the docs discuss
multiranges, since sometimes they only covered rangetypes.

- Instead of an opclass support proc named without_portion, I just
added Set-Returning Functions named range_minus_multi and
multirange_minus_multi, and those are hardcoded for the matching type.
They serve the same purpose: to find the temporal leftovers. If we
wanted to support user-defined types in the future, they could bring
their own SRFs. These functions would also be used for foreign keys
with RESTRICT (depending on how we interpret the standard).

- I abandoned the SPI implementation and went back to just preparing a
TupleTableSlot and calling ExecInsert with it. This was my original
implementation up 'til last year, but I switched to SPI to get correct
trigger behavior. But making triggers do the right thing turned out to
be not so hard after all. See my last email on this thread for lots of
details about how triggers should behave.

- I added tests for protocol tags with FOR PORTION OF. The count from
an update/delete *includes the INSERTs*. This seems consistent with
INSERT ON CONFLICT, which also gives you a count that combines both
inserts and updates. They both have the same mental model (for me at
least) of returning the number of tuples touched. Since FOR PORTION OF
is new, there is no backwards compatibility concern. (Incidentally, I
would love to someday make a protocol change that lets users
distinguish between inserted & updated counts in INSERT ON CONFLICT,
and we could use the same facility to distinguish between
updated/deleted vs inserted in FOR PORTION OF.)

- I did lots of general cleanup in the FOR PORTION OF patch. After 52
versions and many pivots, it had accumulated some bits that didn't
belong there. I split things up a bit more as well: the TriggerData
changes have their own patch now, as do the changes to
FindFKPeriodOpers (prep for CASCADE/SET NULL/SET DEFAULT). I also ran
pgindent on everything.

Rebased to ea06263c4a.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-08-29T13:03:44Z

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-09-25T04:05:41Z

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-10-04T19:48:52Z

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-10-13T06:43:20Z

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-10-24T17:08:26Z

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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2025-10-28T10:49:35Z

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.

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-10-30T06:02:02Z

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-11-04T19:12:46Z

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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2025-11-05T15:46:20Z

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-11-05T16:04:21Z

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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2025-11-12T07:42:07Z

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

Re: SQL:2011 Application Time Update & Delete

Chao Li <li.evan.chao@gmail.com> — 2025-11-12T09:31:53Z


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

Re: SQL:2011 Application Time Update & Delete

Chao Li <li.evan.chao@gmail.com> — 2025-11-13T03:31:57Z


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

Re: SQL:2011 Application Time Update & Delete

Chao Li <li.evan.chao@gmail.com> — 2025-11-13T03:55:35Z

On Nov 5, 2025, at 23:46, Peter Eisentraut <peter@eisentraut.org> wrote:

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


I just noticed 0001 and 0002 have been pushed, and my comments 3&4 on 0002
had been fixed in the pushed version.

So, I created a patch to fix the typo of my comment 1. As the fix is really
trivial, I am fine either merging it or leaving it to Paul for next updates.

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

Re: SQL:2011 Application Time Update & Delete

Chao Li <li.evan.chao@gmail.com> — 2025-11-13T03:56:22Z

Sorry, I missed the attachment.

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


On Thu, Nov 13, 2025 at 11:55 AM Chao Li <li.evan.chao@gmail.com> wrote:

>
>
> On Nov 5, 2025, at 23:46, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I committed the patches 0001 and 0002 (from v59).
>
>
> I just noticed 0001 and 0002 have been pushed, and my comments 3&4 on 0002
> had been fixed in the pushed version.
>
> So, I created a patch to fix the typo of my comment 1. As the fix is
> really trivial, I am fine either merging it or leaving it to Paul for next
> updates.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
>

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-11-13T04:07:49Z

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

Re: SQL:2011 Application Time Update & Delete

Chao Li <li.evan.chao@gmail.com> — 2025-11-14T04:10:00Z


> 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 change to:

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 other places 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 think quote_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/

Re: SQL:2011 Application Time Update & Delete

Chao Li <li.evan.chao@gmail.com> — 2025-11-14T08:38:31Z


> 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 think quote_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 as size 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/

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-11-19T18:49:59Z

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 change to:
>
> 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 other places 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 think quote_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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2025-11-22T08:55:42Z

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.

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-11-26T19:29:30Z

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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2025-11-27T15:44:26Z

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.

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-12-06T00:42:05Z

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2025-12-18T22:41:16Z

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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2026-01-08T16:03:02Z

On 06.12.25 01:42, Paul A Jungwirth wrote:
>> 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.

How about an alternative approach: We record the required constructor 
functions in the pg_range catalog, and then just look them up from 
there.  I have put together a quick patch for this, see attached.

Maybe we don't need to record all of them.  In particular, some of the 
multirange constructor functions seem to only exist to serve as cast 
functions.  Do you foresee down the road needing to look up any other 
ones starting from the range type?

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-01-10T06:16:26Z

On Thu, Jan 8, 2026 at 8:03 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> How about an alternative approach: We record the required constructor
> functions in the pg_range catalog, and then just look them up from
> there.  I have put together a quick patch for this, see attached.

I like this idea!

Patch applies, tests pass.

We would need to document these columns.

> Maybe we don't need to record all of them.  In particular, some of the
> multirange constructor functions seem to only exist to serve as cast
> functions.  Do you foresee down the road needing to look up any other
> ones starting from the range type?

I don't foresee using any of the others. I'm inclined to record all of
them though, in case someone else has a use for them.

And actually I wonder if UPDATE/DELETE FOR PORTION OF should use the
3-arg constructor. We want to guarantee the FROM is inclusive and the
TO is exclusive. That's true for built-in rangetypes, but we should be
explicit to ensure we get the right behavior for other rangetypes too.

```
diff --git a/src/backend/catalog/pg_range.c b/src/backend/catalog/pg_range.c
index cd21c84c8fd..3d194e67fbf 100644
--- a/src/backend/catalog/pg_range.c
+++ b/src/backend/catalog/pg_range.c
@@ -35,7 +35,9 @@
 void
 RangeCreate(Oid rangeTypeOid, Oid rangeSubType, Oid rangeCollation,
             Oid rangeSubOpclass, RegProcedure rangeCanonical,
-            RegProcedure rangeSubDiff, Oid multirangeTypeOid)
+            RegProcedure rangeSubDiff, Oid multirangeTypeOid,
+            RegProcedure rangeConstr2, RegProcedure rangeConstr3,
+            RegProcedure multirangeConstr0, RegProcedure
multirangeConstr1, RegProcedure multirangeConstr2)
 {
     Relation    pg_range;
     Datum        values[Natts_pg_range];
@@ -57,6 +59,11 @@ RangeCreate(Oid rangeTypeOid, Oid rangeSubType, Oid
rangeCollation,
     values[Anum_pg_range_rngcanonical - 1] = ObjectIdGetDatum(rangeCanonical);
     values[Anum_pg_range_rngsubdiff - 1] = ObjectIdGetDatum(rangeSubDiff);
     values[Anum_pg_range_rngmultitypid - 1] =
ObjectIdGetDatum(multirangeTypeOid);
+    values[Anum_pg_range_rngconstr2 - 1] = ObjectIdGetDatum(rangeConstr2);
+    values[Anum_pg_range_rngconstr3 - 1] = ObjectIdGetDatum(rangeConstr3);
+    values[Anum_pg_range_rngmconstr0 - 1] =
ObjectIdGetDatum(multirangeConstr0);
+    values[Anum_pg_range_rngmconstr1 - 1] =
ObjectIdGetDatum(multirangeConstr1);
+    values[Anum_pg_range_rngmconstr2 - 1] =
ObjectIdGetDatum(multirangeConstr2);

     tup = heap_form_tuple(RelationGetDescr(pg_range), values, nulls);

```

The C code uses `mltrng` a lot. Do we want to use that here? I don't
see it in the catalog yet, but it seems clearer than `rngm`. I guess
we have to start with `rng` though. We have `rngmultitypid`, so maybe
`rngmulticonstr0`? Okay I understand why you went with `rngm`.

It's tempting to use two oidvectors, one for range constructors and
another for multirange, with the 0-arg constructor in position 0,
1-arg in position 1, etc. We could use InvalidOid to say there is no
such constructor. So we would have rngconstr of `{0,0,123,456}` and
mltrngconstr of `{123,456,789}`. But is it better to avoid varlena
columns if we can?

```
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index e5fa0578889..0a92688b298 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -111,10 +111,12 @@ Oid
binary_upgrade_next_mrng_pg_type_oid = InvalidOid;
 Oid            binary_upgrade_next_mrng_array_pg_type_oid = InvalidOid;

 static void makeRangeConstructors(const char *name, Oid namespace,
-                                  Oid rangeOid, Oid subtype);
+                                  Oid rangeOid, Oid subtype,
+                                  Oid rangeConstrOids[]);
 static void makeMultirangeConstructors(const char *name, Oid namespace,
                                        Oid multirangeOid, Oid rangeOid,
-                                       Oid rangeArrayOid, Oid *castFuncOid);
+                                       Oid rangeArrayOid, Oid *castFuncOid,
+                                       Oid multirangeConstrOids[]);
 static Oid    findTypeInputFunction(List *procname, Oid typeOid);
 static Oid    findTypeOutputFunction(List *procname, Oid typeOid);
 static Oid    findTypeReceiveFunction(List *procname, Oid typeOid);
@@ -1406,6 +1408,8 @@ DefineRange(ParseState *pstate, CreateRangeStmt *stmt)
     ListCell   *lc;
     ObjectAddress address;
     ObjectAddress mltrngaddress PG_USED_FOR_ASSERTS_ONLY;
+    Oid            rangeConstrOids[2];
+    Oid            multirangeConstrOids[3];
     Oid            castFuncOid;

     /* Convert list of names to a name and namespace */
@@ -1661,10 +1665,6 @@ DefineRange(ParseState *pstate, CreateRangeStmt *stmt)
                    InvalidOid); /* type's collation (ranges never have one) */
     Assert(multirangeOid == mltrngaddress.objectId);

-    /* Create the entry in pg_range */
-    RangeCreate(typoid, rangeSubtype, rangeCollation, rangeSubOpclass,
-                rangeCanonical, rangeSubtypeDiff, multirangeOid);
-
     /*
      * Create the array type that goes with it.
      */
@@ -1746,10 +1746,16 @@ DefineRange(ParseState *pstate, CreateRangeStmt *stmt)
     CommandCounterIncrement();

     /* And create the constructor functions for this range type */
-    makeRangeConstructors(typeName, typeNamespace, typoid, rangeSubtype);
+    makeRangeConstructors(typeName, typeNamespace, typoid,
rangeSubtype, rangeConstrOids);
     makeMultirangeConstructors(multirangeTypeName, typeNamespace,
                                multirangeOid, typoid, rangeArrayOid,
-                               &castFuncOid);
+                               &castFuncOid, multirangeConstrOids);
+
+    /* Create the entry in pg_range */
+    RangeCreate(typoid, rangeSubtype, rangeCollation, rangeSubOpclass,
+                rangeCanonical, rangeSubtypeDiff, multirangeOid,
+                rangeConstrOids[0], rangeConstrOids[1],
+                multirangeConstrOids[0], multirangeConstrOids[1],
multirangeConstrOids[2]);

     /* Create cast from the range type to its multirange type */
     CastCreate(typoid, multirangeOid, castFuncOid, InvalidOid, InvalidOid,
@@ -1772,7 +1778,8 @@ DefineRange(ParseState *pstate, CreateRangeStmt *stmt)
  */
 static void
 makeRangeConstructors(const char *name, Oid namespace,
-                      Oid rangeOid, Oid subtype)
+                      Oid rangeOid, Oid subtype,
+                      Oid rangeConstrOids[])
 {
     static const char *const prosrc[2] = {"range_constructor2",
     "range_constructor3"};
@@ -1833,6 +1840,8 @@ makeRangeConstructors(const char *name, Oid namespace,
          * pg_dump depends on this choice to avoid dumping the constructors.
          */
         recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
+
+        rangeConstrOids[i] = myself.objectId;
     }
 }

@@ -1848,7 +1857,7 @@ makeRangeConstructors(const char *name, Oid namespace,
 static void
 makeMultirangeConstructors(const char *name, Oid namespace,
                            Oid multirangeOid, Oid rangeOid, Oid rangeArrayOid,
-                           Oid *castFuncOid)
+                           Oid *castFuncOid, Oid multirangeConstrOids[])
 {
     ObjectAddress myself,
                 referenced;
@@ -1899,6 +1908,7 @@ makeMultirangeConstructors(const char *name, Oid
namespace,
      * depends on this choice to avoid dumping the constructors.
      */
     recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
+    multirangeConstrOids[0] = myself.objectId;
     pfree(argtypes);

     /*
@@ -1939,6 +1949,7 @@ makeMultirangeConstructors(const char *name, Oid
namespace,
                              0.0);    /* prorows */
     /* ditto */
     recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
+    multirangeConstrOids[1] = myself.objectId;
     pfree(argtypes);
     *castFuncOid = myself.objectId;

@@ -1978,6 +1989,7 @@ makeMultirangeConstructors(const char *name, Oid
namespace,
                              0.0);    /* prorows */
     /* ditto */
     recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
+    multirangeConstrOids[2] = myself.objectId;
     pfree(argtypes);
     pfree(allParameterTypes);
     pfree(parameterModes);
```

This all looks good to me.

```
diff --git a/src/include/catalog/pg_range.dat b/src/include/catalog/pg_range.dat
index 830971c4944..f1e46a9d830 100644
--- a/src/include/catalog/pg_range.dat
+++ b/src/include/catalog/pg_range.dat
@@ -14,21 +14,33 @@

 { rngtypid => 'int4range', rngsubtype => 'int4',
   rngmultitypid => 'int4multirange', rngsubopc => 'btree/int4_ops',
+  rngconstr2 => 'int4range(int4,int4)', rngconstr3 =>
'int4range(int4,int4,text)',
+  rngmconstr0 => 'int4multirange()', rngmconstr1 =>
'int4multirange(int4range)', rngmconstr2 =>
'int4multirange(_int4range)',
   rngcanonical => 'int4range_canonical', rngsubdiff => 'int4range_subdiff' },
 { rngtypid => 'numrange', rngsubtype => 'numeric',
   rngmultitypid => 'nummultirange', rngsubopc => 'btree/numeric_ops',
+  rngconstr2 => 'numrange(numeric,numeric)', rngconstr3 =>
'numrange(numeric,numeric,text)',
+  rngmconstr0 => 'nummultirange()', rngmconstr1 =>
'nummultirange(numrange)', rngmconstr2 => 'nummultirange(_numrange)',
   rngcanonical => '-', rngsubdiff => 'numrange_subdiff' },
 { rngtypid => 'tsrange', rngsubtype => 'timestamp',
   rngmultitypid => 'tsmultirange', rngsubopc => 'btree/timestamp_ops',
+  rngconstr2 => 'tsrange(timestamp,timestamp)', rngconstr3 =>
'tsrange(timestamp,timestamp,text)',
+  rngmconstr0 => 'tsmultirange()', rngmconstr1 =>
'tsmultirange(tsrange)', rngmconstr2 => 'tsmultirange(_tsrange)',
   rngcanonical => '-', rngsubdiff => 'tsrange_subdiff' },
 { rngtypid => 'tstzrange', rngsubtype => 'timestamptz',
   rngmultitypid => 'tstzmultirange', rngsubopc => 'btree/timestamptz_ops',
+  rngconstr2 => 'tstzrange(timestamptz,timestamptz)', rngconstr3 =>
'tstzrange(timestamptz,timestamptz,text)',
+  rngmconstr0 => 'tstzmultirange()', rngmconstr1 =>
'tstzmultirange(tstzrange)', rngmconstr2 =>
'tstzmultirange(_tstzrange)',
   rngcanonical => '-', rngsubdiff => 'tstzrange_subdiff' },
 { rngtypid => 'daterange', rngsubtype => 'date',
   rngmultitypid => 'datemultirange', rngsubopc => 'btree/date_ops',
+  rngconstr2 => 'daterange(date,date)', rngconstr3 =>
'daterange(date,date,text)',
+  rngmconstr0 => 'datemultirange()', rngmconstr1 =>
'datemultirange(daterange)', rngmconstr2 =>
'datemultirange(_daterange)',
   rngcanonical => 'daterange_canonical', rngsubdiff => 'daterange_subdiff' },
 { rngtypid => 'int8range', rngsubtype => 'int8',
   rngmultitypid => 'int8multirange', rngsubopc => 'btree/int8_ops',
+  rngconstr2 => 'int8range(int8,int8)', rngconstr3 =>
'int8range(int8,int8,text)',
+  rngmconstr0 => 'int8multirange()', rngmconstr1 =>
'int8multirange(int8range)', rngmconstr2 =>
'int8multirange(_int8range)',
   rngcanonical => 'int8range_canonical', rngsubdiff => 'int8range_subdiff' },

 ]
```

Do the .dat files have a way to set oidvector columns?

```
diff --git a/src/include/catalog/pg_range.h b/src/include/catalog/pg_range.h
index 5b4f4615905..ad4d1e9187f 100644
--- a/src/include/catalog/pg_range.h
+++ b/src/include/catalog/pg_range.h
@@ -43,6 +43,15 @@ CATALOG(pg_range,3541,RangeRelationId)
     /* subtype's btree opclass */
     Oid            rngsubopc BKI_LOOKUP(pg_opclass);

+    /* range constructor functions */
+    regproc        rngconstr2 BKI_LOOKUP(pg_proc);
+    regproc        rngconstr3 BKI_LOOKUP(pg_proc);
+
+    /* multirange constructor functions */
+    regproc        rngmconstr0 BKI_LOOKUP(pg_proc);
+    regproc        rngmconstr1 BKI_LOOKUP(pg_proc);
+    regproc        rngmconstr2 BKI_LOOKUP(pg_proc);
+
     /* canonicalize range, or 0 */
     regproc        rngcanonical BKI_LOOKUP_OPT(pg_proc);
```

Is there a reason you're adding them in the middle of the struct? It
doesn't help with packing.

```
diff --git a/src/test/regress/sql/type_sanity.sql
b/src/test/regress/sql/type_sanity.sql
index c2496823d90..1a1bd3f14a7 100644
--- a/src/test/regress/sql/type_sanity.sql
+++ b/src/test/regress/sql/type_sanity.sql
...
```

I like the tests you've added here.

This needs some kind of pg_upgrade support I assume? It will have to
work for user-defined rangetypes too. So I guess we would still need
some code like what's in my patch, although keeping it just for the
v18 -> v19 upgrade seems better than having it in core indefinitely.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2026-01-19T13:37:43Z

On 10.01.26 07:16, Paul A Jungwirth wrote:
> We would need to document these columns.

Done that.

> The C code uses `mltrng` a lot. Do we want to use that here? I don't
> see it in the catalog yet, but it seems clearer than `rngm`. I guess
> we have to start with `rng` though. We have `rngmultitypid`, so maybe
> `rngmulticonstr0`? Okay I understand why you went with `rngm`.

I tuned the naming again in the new patch.  I changed "constr" to 
"construct" because "constr" read too much like "constraint" to me.  I 
also did a bit of "mtlrng".  I think it's a bit more consistent and less 
ambiguous now.

> It's tempting to use two oidvectors, one for range constructors and
> another for multirange, with the 0-arg constructor in position 0,
> 1-arg in position 1, etc. We could use InvalidOid to say there is no
> such constructor. So we would have rngconstr of `{0,0,123,456}` and
> mltrngconstr of `{123,456,789}`. But is it better to avoid varlena
> columns if we can?

I don't think oidvectors would be appropriate here.  These are for when 
you have a group of values that you need together, like for function 
arguments.  But here we want to access them separately.  And it would 
create a lot of notational and a bit of storage overhead.

I had in the previous patch used some arrays as arguments in the 
internal functions, but in the second patch I'm also getting rid of that 
because it's uselessly inconsistent.

> ```
> diff --git a/src/include/catalog/pg_range.h b/src/include/catalog/pg_range.h
> index 5b4f4615905..ad4d1e9187f 100644
> --- a/src/include/catalog/pg_range.h
> +++ b/src/include/catalog/pg_range.h
> @@ -43,6 +43,15 @@ CATALOG(pg_range,3541,RangeRelationId)
>       /* subtype's btree opclass */
>       Oid            rngsubopc BKI_LOOKUP(pg_opclass);
> 
> +    /* range constructor functions */
> +    regproc        rngconstr2 BKI_LOOKUP(pg_proc);
> +    regproc        rngconstr3 BKI_LOOKUP(pg_proc);
> +
> +    /* multirange constructor functions */
> +    regproc        rngmconstr0 BKI_LOOKUP(pg_proc);
> +    regproc        rngmconstr1 BKI_LOOKUP(pg_proc);
> +    regproc        rngmconstr2 BKI_LOOKUP(pg_proc);
> +
>       /* canonicalize range, or 0 */
>       regproc        rngcanonical BKI_LOOKUP_OPT(pg_proc);
> ```
> 
> Is there a reason you're adding them in the middle of the struct? It
> doesn't help with packing.

Well, initially I had done that so that the edits to pg_range.dat are 
easier.  But I think this order makes some sense, because it has the 
mandatory data first and then the optional data later.  But it doesn't 
matter much either way.

> This needs some kind of pg_upgrade support I assume? It will have to
> work for user-defined rangetypes too.

No, I don't think there needs to be pg_upgrade support.  Existing range 
types are dumped as CREATE TYPE ... RANGE commands, and when those get 
restored it will create the new catalog entries.

Re: SQL:2011 Application Time Update & Delete

Kirill Reshke <reshkekirill@gmail.com> — 2026-01-19T17:43:44Z

On Mon, 19 Jan 2026 at 18:37, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 10.01.26 07:16, Paul A Jungwirth wrote:
> > We would need to document these columns.
>
> Done that.
>
> > The C code uses `mltrng` a lot. Do we want to use that here? I don't
> > see it in the catalog yet, but it seems clearer than `rngm`. I guess
> > we have to start with `rng` though. We have `rngmultitypid`, so maybe
> > `rngmulticonstr0`? Okay I understand why you went with `rngm`.
>
> I tuned the naming again in the new patch.  I changed "constr" to
> "construct" because "constr" read too much like "constraint" to me.  I
> also did a bit of "mtlrng".  I think it's a bit more consistent and less
> ambiguous now.
>
> > It's tempting to use two oidvectors, one for range constructors and
> > another for multirange, with the 0-arg constructor in position 0,
> > 1-arg in position 1, etc. We could use InvalidOid to say there is no
> > such constructor. So we would have rngconstr of `{0,0,123,456}` and
> > mltrngconstr of `{123,456,789}`. But is it better to avoid varlena
> > columns if we can?
>
> I don't think oidvectors would be appropriate here.  These are for when
> you have a group of values that you need together, like for function
> arguments.  But here we want to access them separately.  And it would
> create a lot of notational and a bit of storage overhead.
>
> I had in the previous patch used some arrays as arguments in the
> internal functions, but in the second patch I'm also getting rid of that
> because it's uselessly inconsistent.
>
> > ```
> > diff --git a/src/include/catalog/pg_range.h b/src/include/catalog/pg_range.h
> > index 5b4f4615905..ad4d1e9187f 100644
> > --- a/src/include/catalog/pg_range.h
> > +++ b/src/include/catalog/pg_range.h
> > @@ -43,6 +43,15 @@ CATALOG(pg_range,3541,RangeRelationId)
> >       /* subtype's btree opclass */
> >       Oid            rngsubopc BKI_LOOKUP(pg_opclass);
> >
> > +    /* range constructor functions */
> > +    regproc        rngconstr2 BKI_LOOKUP(pg_proc);
> > +    regproc        rngconstr3 BKI_LOOKUP(pg_proc);
> > +
> > +    /* multirange constructor functions */
> > +    regproc        rngmconstr0 BKI_LOOKUP(pg_proc);
> > +    regproc        rngmconstr1 BKI_LOOKUP(pg_proc);
> > +    regproc        rngmconstr2 BKI_LOOKUP(pg_proc);
> > +
> >       /* canonicalize range, or 0 */
> >       regproc        rngcanonical BKI_LOOKUP_OPT(pg_proc);
> > ```
> >
> > Is there a reason you're adding them in the middle of the struct? It
> > doesn't help with packing.
>
> Well, initially I had done that so that the edits to pg_range.dat are
> easier.  But I think this order makes some sense, because it has the
> mandatory data first and then the optional data later.  But it doesn't
> matter much either way.
>
> > This needs some kind of pg_upgrade support I assume? It will have to
> > work for user-defined rangetypes too.
>
> No, I don't think there needs to be pg_upgrade support.  Existing range
> types are dumped as CREATE TYPE ... RANGE commands, and when those get
> restored it will create the new catalog entries.

Hi!
I have looked into v2. This patch looks good. Making explicit links in
pg_catalog seems to be more cve-proof to me. Using Paul's approach
(get_typname_and_namespace) is not only fragile, it is a recipe for
CVE if any mistake is made, is it? I mean, matching something by name
is vulnerable for search-path-based CVE (again, not saying this is the
case in Paul patch).

I think patch tests are good. Also, I don't think we need to mention
any "upcoming patches" in the commit message - this change has its own
value.

One stupid question from me: should we add

````
 t.typanalyze!='range_typanalyze'::regproc or t.typinput !=
'range_in'::regproc or t.typoutput != 'range_out'::regproc  or
t.typreceive != 'range_recv'::regproc or typsend !=
'range_send'::regproc;

````


 In type sanity sql check? In my understanding, this condition
(t.typanalyze == 'range_typanalyze'::regproc and ....)  is required
for built-in range types, and for user-defined seems to also be true.
-- 
Best regards,
Kirill Reshke

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-01-19T18:33:02Z

On Mon, Jan 19, 2026 at 5:37 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I tuned the naming again in the new patch.  I changed "constr" to
> "construct" because "constr" read too much like "constraint" to me.  I
> also did a bit of "mtlrng".  I think it's a bit more consistent and less
> ambiguous now.

I agree that seems like an improvement.

> > It's tempting to use two oidvectors, one for range constructors and
> > another for multirange, with the 0-arg constructor in position 0,
> > 1-arg in position 1, etc. We could use InvalidOid to say there is no
> > such constructor. So we would have rngconstr of `{0,0,123,456}` and
> > mltrngconstr of `{123,456,789}`. But is it better to avoid varlena
> > columns if we can?
>
> I don't think oidvectors would be appropriate here.  These are for when
> you have a group of values that you need together, like for function
> arguments.  But here we want to access them separately.  And it would
> create a lot of notational and a bit of storage overhead.

Okay.

> > Is there a reason you're adding them in the middle of the struct? It
> > doesn't help with packing.
>
> Well, initially I had done that so that the edits to pg_range.dat are
> easier.  But I think this order makes some sense, because it has the
> mandatory data first and then the optional data later.  But it doesn't
> matter much either way.

Okay. And ABI compatibility is only between minor versions, so no concern there.

> > This needs some kind of pg_upgrade support I assume? It will have to
> > work for user-defined rangetypes too.
>
> No, I don't think there needs to be pg_upgrade support.  Existing range
> types are dumped as CREATE TYPE ... RANGE commands, and when those get
> restored it will create the new catalog entries.

Okay, that's great!

Do we want a regress test in rangetypes.sql to confirm that these are
set correctly (especially for user-defined types)? I checked manually
after `make installcheck`, and they look fine, but should it be in our
test suite?

Here is another thought I had: As we've talked about in the
application-time threads, I would like temporal features to be
extensible enough to support user-defined types. We almost achieve
that, but we need something like a "type support function". For primary
key and unique constraints, we need a way to reject invalid values like
empty ranges. For foreign keys we need an intersect operator (which is
not currently in pg_amop, since it is neither for search nor ordering,
and isn't involved in indexes anyway). And for UPDATE/DELETE FOR
PORTION OF we need a foo_minus_multi to compute the "temporal
leftovers".

We could also ask for a constructor function, to build the targeted
portion from the FROM/TO bounds. This is not strictly necessary, since
we also have the FOR PORTION OF valid_at (...) syntax (which is used by
multiranges). But it's something that would be nice to offer. In that
case range types would not need these extra columns in pg_range.

But recording the constructor oids in pg_range still has inherent
value, and doing it now doesn't *prevent* us from later adding a
facility to get a constructor function for FOR PORTION OF bounds. So I
don't think there is any downside to recording them here.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2026-01-22T15:21:54Z

I have committed the pg_range patch.

On 19.01.26 19:33, Paul A Jungwirth wrote:
> Do we want a regress test in rangetypes.sql to confirm that these are
> set correctly (especially for user-defined types)? I checked manually
> after `make installcheck`, and they look fine, but should it be in our
> test suite?

I think the existing tests do that, since type_sanity runs after the 
rangetypes test.

> Here is another thought I had: As we've talked about in the
> application-time threads, I would like temporal features to be
> extensible enough to support user-defined types. We almost achieve
> that, but we need something like a "type support function". For primary
> key and unique constraints, we need a way to reject invalid values like
> empty ranges. For foreign keys we need an intersect operator (which is
> not currently in pg_amop, since it is neither for search nor ordering,
> and isn't involved in indexes anyway). And for UPDATE/DELETE FOR
> PORTION OF we need a foo_minus_multi to compute the "temporal
> leftovers".
> 
> We could also ask for a constructor function, to build the targeted
> portion from the FROM/TO bounds. This is not strictly necessary, since
> we also have the FOR PORTION OF valid_at (...) syntax (which is used by
> multiranges). But it's something that would be nice to offer. In that
> case range types would not need these extra columns in pg_range.
> 
> But recording the constructor oids in pg_range still has inherent
> value, and doing it now doesn't *prevent* us from later adding a
> facility to get a constructor function for FOR PORTION OF bounds. So I
> don't think there is any downside to recording them here.

Right, that sounds like a future project.

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2026-01-22T15:23:01Z

On 19.01.26 18:43, Kirill Reshke wrote:
> One stupid question from me: should we add
> 
> ````
>   t.typanalyze!='range_typanalyze'::regproc or t.typinput !=
> 'range_in'::regproc or t.typoutput != 'range_out'::regproc  or
> t.typreceive != 'range_recv'::regproc or typsend !=
> 'range_send'::regproc;
> 
> ````

Maybe, but this seems to be outside of this patch.  There are also 
similar considerations for arrays, domains, etc.

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-02-11T21:25:21Z

On Thu, Jan 22, 2026 at 7:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I have committed the pg_range patch.

Thanks! Here are v65 patches for UPDATE/DELETE FOR PORTION OF. I kept
the get_range_constructor2 helper function as a separate patch, but it
probably doesn't really need to be a separate commit. Maybe it could
even be inlined into its caller.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-02-13T20:00:52Z

On Wed, Feb 11, 2026 at 1:25 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Thu, Jan 22, 2026 at 7:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > I have committed the pg_range patch.
>
> Thanks! Here are v65 patches for UPDATE/DELETE FOR PORTION OF. I kept
> the get_range_constructor2 helper function as a separate patch, but it
> probably doesn't really need to be a separate commit. Maybe it could
> even be inlined into its caller.

Here is another round to fix a few rebase conflicts.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-02-20T17:16:11Z

On Fri, Feb 13, 2026 at 12:00 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Here is another round to fix a few rebase conflicts.

I realized we didn't have any tests for v18's new feature to say
`UPDATE ... RETURNING OLD.foo, NEW.foo`. These patches add a small
test for `RETURNING OLD.valid_at, NEW.valid_at` when you say `UPDATE
FOR PORTION OF valid_at`. This seems worth testing since that column
gets set in an automatic way, not via the normal SET syntax. No fixes
were needed.

I also corrected the commit message, which still referred to the
without_overlaps function that we renamed to
{range,multirange}_minus_multi.

As far as I know nothing else here is waiting on me, but please
correct me if I've overlooked something.

Rebased to 18bcdb75d1.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Kirill Reshke <reshkekirill@gmail.com> — 2026-03-10T12:26:30Z

On Fri, 20 Feb 2026 at 22:16, Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Fri, Feb 13, 2026 at 12:00 PM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > Here is another round to fix a few rebase conflicts.
>
> I realized we didn't have any tests for v18's new feature to say
> `UPDATE ... RETURNING OLD.foo, NEW.foo`. These patches add a small
> test for `RETURNING OLD.valid_at, NEW.valid_at` when you say `UPDATE
> FOR PORTION OF valid_at`. This seems worth testing since that column
> gets set in an automatic way, not via the normal SET syntax. No fixes
> were needed.
>
> I also corrected the commit message, which still referred to the
> without_overlaps function that we renamed to
> {range,multirange}_minus_multi.
>
> As far as I know nothing else here is waiting on me, but please
> correct me if I've overlooked something.
>
> Rebased to 18bcdb75d1.
>
> Yours,
>
> --
> Paul              ~{:-)
> pj@illuminatedcomputing.com

Hi!
v67-0001 looks good to me.

When applying first two of patches from v67 series, my initdb fails:

```
reshke@yezzey-cbdb-bench:~/cpg$ ./bin/bin/initdb -D ./db
The files belonging to this database system will be owned by user "reshke".
This user must also own the server process.

The database cluster will be initialized with locale "C.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are enabled.

creating directory db ... ok
creating subdirectories ... ok
selecting dynamic shared memory implementation ... posix
selecting default "max_connections" ... 100
selecting default "shared_buffers" ... 128MB
selecting default time zone ... Etc/UTC
creating configuration files ... ok
running bootstrap script ... ok
performing post-bootstrap initialization ... 2026-03-10 12:21:05.842
UTC [2995664] WARNING:  unrecognized node type: 155
2026-03-10 12:21:05.842 UTC [2995664] FATAL:  unrecognized node type: 155
2026-03-10 12:21:05.842 UTC [2995664] STATEMENT:  REVOKE ALL ON
pg_authid FROM public;

child process exited with exit code 1
initdb: removing data directory "db"
```

without v67-0002 initdb runs ok.

Also, after v67-0002 my createdb fails:

```
reshke@yezzey-cbdb-bench:~/cpg$ ./bin/bin/createdb
createdb: error: query failed: ERROR:  syntax error at or near "("
LINE 1: SELECT pg_catalog.set_config('search_path', '', false);
                                    ^
createdb: detail: Query was: SELECT
pg_catalog.set_config('search_path', '', false);
```

Simple queries also fails:
```
postgres=# select now();
WARNING:  unrecognized node type: 144
ERROR:  unrecognized node type: 76
```

--
Best regards,
Kirill Reshke

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-03-10T16:13:00Z

On Tue, Mar 10, 2026 at 5:26 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
> When applying first two of patches from v67 series, my initdb fails:
>
> ```
> reshke@yezzey-cbdb-bench:~/cpg$ ./bin/bin/initdb -D ./db
> The files belonging to this database system will be owned by user "reshke".
> This user must also own the server process.
>
> The database cluster will be initialized with locale "C.UTF-8".
> The default database encoding has accordingly been set to "UTF8".
> The default text search configuration will be set to "english".
>
> Data page checksums are enabled.
>
> creating directory db ... ok
> creating subdirectories ... ok
> selecting dynamic shared memory implementation ... posix
> selecting default "max_connections" ... 100
> selecting default "shared_buffers" ... 128MB
> selecting default time zone ... Etc/UTC
> creating configuration files ... ok
> running bootstrap script ... ok
> performing post-bootstrap initialization ... 2026-03-10 12:21:05.842
> UTC [2995664] WARNING:  unrecognized node type: 155
> 2026-03-10 12:21:05.842 UTC [2995664] FATAL:  unrecognized node type: 155
> 2026-03-10 12:21:05.842 UTC [2995664] STATEMENT:  REVOKE ALL ON
> pg_authid FROM public;
>
> child process exited with exit code 1
> initdb: removing data directory "db"
> ```
>
> without v67-0002 initdb runs ok.
>
> Also, after v67-0002 my createdb fails:
>
> ```
> reshke@yezzey-cbdb-bench:~/cpg$ ./bin/bin/createdb
> createdb: error: query failed: ERROR:  syntax error at or near "("
> LINE 1: SELECT pg_catalog.set_config('search_path', '', false);
>                                     ^
> createdb: detail: Query was: SELECT
> pg_catalog.set_config('search_path', '', false);
> ```
>
> Simple queries also fails:
> ```
> postgres=# select now();
> WARNING:  unrecognized node type: 144
> ERROR:  unrecognized node type: 76
> ```

I don't see any of these problems here (after an error-free rebase
onto a198c26ded), and CI passes. Are you sure that was from a clean
build? If so, could you share your configure line?

Thanks,

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

Re: SQL:2011 Application Time Update & Delete

Kirill Reshke <reshkekirill@gmail.com> — 2026-03-10T17:33:30Z

On Tue, 10 Mar 2026 at 21:13, Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Tue, Mar 10, 2026 at 5:26 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
> > When applying first two of patches from v67 series, my initdb fails:
> >
> > ```
> > reshke@yezzey-cbdb-bench:~/cpg$ ./bin/bin/initdb -D ./db
> > The files belonging to this database system will be owned by user "reshke".
> > This user must also own the server process.
> >
> > The database cluster will be initialized with locale "C.UTF-8".
> > The default database encoding has accordingly been set to "UTF8".
> > The default text search configuration will be set to "english".
> >
> > Data page checksums are enabled.
> >
> > creating directory db ... ok
> > creating subdirectories ... ok
> > selecting dynamic shared memory implementation ... posix
> > selecting default "max_connections" ... 100
> > selecting default "shared_buffers" ... 128MB
> > selecting default time zone ... Etc/UTC
> > creating configuration files ... ok
> > running bootstrap script ... ok
> > performing post-bootstrap initialization ... 2026-03-10 12:21:05.842
> > UTC [2995664] WARNING:  unrecognized node type: 155
> > 2026-03-10 12:21:05.842 UTC [2995664] FATAL:  unrecognized node type: 155
> > 2026-03-10 12:21:05.842 UTC [2995664] STATEMENT:  REVOKE ALL ON
> > pg_authid FROM public;
> >
> > child process exited with exit code 1
> > initdb: removing data directory "db"
> > ```
> >
> > without v67-0002 initdb runs ok.
> >
> > Also, after v67-0002 my createdb fails:
> >
> > ```
> > reshke@yezzey-cbdb-bench:~/cpg$ ./bin/bin/createdb
> > createdb: error: query failed: ERROR:  syntax error at or near "("
> > LINE 1: SELECT pg_catalog.set_config('search_path', '', false);
> >                                     ^
> > createdb: detail: Query was: SELECT
> > pg_catalog.set_config('search_path', '', false);
> > ```
> >
> > Simple queries also fails:
> > ```
> > postgres=# select now();
> > WARNING:  unrecognized node type: 144
> > ERROR:  unrecognized node type: 76
> > ```
>
> I don't see any of these problems here (after an error-free rebase
> onto a198c26ded), and CI passes. Are you sure that was from a clean
> build? If so, could you share your configure line?
>
> Thanks,
>
> --
> Paul              ~{:-)
> pj@illuminatedcomputing.com

Sorry, It was indeed an issue on my side. It all gone after make
maintainer-clean.
Anyway, I was interested in by-hand testing of 0001 & 0002, which I
did. I tested various partitioned table use-cases, including the new
MERGE PARTITIONS feature, updating the partition column, etc. All
seems to work just fine.

The only review comment I have is that we may need tab-completion
support for UPDATE ... [FOR PARTITION OF] pattern.

-- 
Best regards,
Kirill Reshke

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2026-03-12T08:38:56Z

On 20.02.26 18:16, Paul A Jungwirth wrote:
> On Fri, Feb 13, 2026 at 12:00 PM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
>>
>> Here is another round to fix a few rebase conflicts.
> 
> I realized we didn't have any tests for v18's new feature to say
> `UPDATE ... RETURNING OLD.foo, NEW.foo`. These patches add a small
> test for `RETURNING OLD.valid_at, NEW.valid_at` when you say `UPDATE
> FOR PORTION OF valid_at`. This seems worth testing since that column
> gets set in an automatic way, not via the normal SET syntax. No fixes
> were needed.
> 
> I also corrected the commit message, which still referred to the
> without_overlaps function that we renamed to
> {range,multirange}_minus_multi.
> 
> As far as I know nothing else here is waiting on me, but please
> correct me if I've overlooked something.
> 
> Rebased to 18bcdb75d1.

Hi Paul,

Review of v67-0001-Add-range_get_constructor2-to-lsyscache.patch and
v67-0002-Add-UPDATE-DELETE-FOR-PORTION-OF.patch:

1) In src/backend/parser/analyze.c, transformForPortionOfClause():

The variable range_name is not very useful; using
forPortionOf->range_name is clearer.

Try making the forPortionOf argument const.

The initialization strat = RTOverlapStrategyNumber; is confusing.

2) src/backend/parser/gram.y

Explain the %prec in opt_alias with a comment.  Maybe the production
name should be more specific if it's only applicable to some specific
statement.

3) In the test src/test/regress/expected/for_portion_of.out:

3.1)

Fix the wording of the error message here (remove "period", add back in 
later patch):

+ERROR:  column or period "invalid_at" of relation "for_portion_of_test" 
does not exist

3.2)

Could this error message have a location pointer?

+ERROR:  range lower bound must be less than or equal to range upper bound

3.3)

Improve wording of this error message:

+ERROR:  got a NULL FOR PORTION OF target

("FOR PORTION OF target was null"?)

3.4)

+-- Updating the non-range part of the PK:

This test updates the id column but the SELECT afterwards only shows
rows for the old id value.  The SELECT ought to use something like

WHERE id = '[1,2)' OR id = '[6,7)'

3.5)

-- UPDATE FOR PORTION OF in a CTE:

I think this test is meant to show that the UPDATE FOR PORTION OF
... RETURNING returns only the updated rows, not the inserted
"leftovers".

First, it would be good if the comment was more clear about what it's
trying to demonstrate.

And then, is there a reason for this behavior versus the alternatives?
SQL standard, other implementations?

3.6)

+-- Not visible to UPDATE:
+-- Tuples updated/inserted within the CTE are not visible to the main 
query yet,
+-- but neither are old tuples the CTE changed:

Is this behavior the same or different from the way normal queries
work?  Could be clarified in the comment either way.

3.7)

+-- UPDATE ... RETURNING returns only the updated values (not the 
inserted side values)

This test looks redundant with earlier tests.  Otherwise, maybe add a
comment about how it's different.

3.8)

+-- test that we run triggers on the UPDATE/DELETEd row and the INSERTed 
rows

Please indent the non-first rows of the CREATE TRIGGER statements.

4) In src/test/subscription/t/034_temporal.pl

+[4,5)|[2000-01-01,2010-01-01)|a}, 'replicated temporal_unique DEFAULT');

The change from FULL to DEFAULT seems wrong.

5) NULL bounds

A general comment: In particular after studying these tests in detail,
I'm suspicious that it's a good idea to interpret null bounds as
unbounded.  Expressions could return null for nested reasons, it would
be very hard to follow that.  Null values should mean "unknown",
unbounded should be explicit.  We have the keyword UNBOUNDED already,
maybe you could use that?  Or do you want to be able to return
unboundedness from an expression?

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-03-13T17:06:50Z

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

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

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

Okay, done.

> Try making the forPortionOf argument const.

Done.

> The initialization strat = RTOverlapStrategyNumber; is confusing.

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

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

Added an explanation and renamed the production.

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

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

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

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

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

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

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

Okay, done.

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

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

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

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

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

> 3.7)
>
> +-- UPDATE ... RETURNING returns only the updated values (not the
> inserted side values)
>
> This test looks redundant with earlier tests.  Otherwise, maybe add a
> comment about how it's different.

I don't think a top-level RETURNING test is redundant with the CTE
test. I expanded the comment here a bit to clarify the goal. It
addresses your question above: Should RETURNING include the inserted
leftovers? I don't think that makes sense:

1. Our docs say, "The optional RETURNING clause causes UPDATE to
compute and return value(s) based on each row actually updated." The
leftovers were not updated.

2. Conceptually, the leftovers represent what *didn't* change.

3. If you implemented this with a trigger, you also wouldn't get the
inserted leftovers.

4. The SQL standard doesn't have RETURNING. But it does say that to
insert the leftovers the system should execute a separate insert
"statement". So we should do something very similar to the trigger
case.

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

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

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

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

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

Done.

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

You're right; fixed.

> 5) NULL bounds
>
> A general comment: In particular after studying these tests in detail,
> I'm suspicious that it's a good idea to interpret null bounds as
> unbounded.  Expressions could return null for nested reasons, it would
> be very hard to follow that.  Null values should mean "unknown",
> unbounded should be explicit.  We have the keyword UNBOUNDED already,
> maybe you could use that?  Or do you want to be able to return
> unboundedness from an expression?

I like the idea of a keyword. I tried adding UNBOUNDED but it caused a
few hundred S/R and R/R conflicts that I couldn't easily resolve. A
year or two ago I had keywords here (MINVALUE/MAXVALUE IIRC), but it
required some nasty parser hacks. This is a pretty delicate area of
the grammar, because we have a_expr with FROM and TO and no
punctuation. I'm already doing some contortions to handle `FOR PORTION
OF valid_at FROM t1 + INTERVAL '1' YEAR TO MONTH TO t2`.

A keyword is not offered by the standard here, so it would just be
custom syntactic sugar. No other RDBMS has one (I think).

I think NULL is the right choice for unbounded. It is what range types
use, and we want this to mesh well with them. More important it works
for *any type*. We don't always have +/-Infinity.

Also I think we should expand user choice rather than restrict it. If
users want to forbid nulls, they can (e.g. by using a domain type).
But if we forbid it, there is no way to override that decision.

Going back to the UNBOUNDED keyword: if we forbid nulls, then a
keyword doesn't really add clarity, since users would already say
`-Infinity` or `Infinity`. It's really just a way to express what null
means in this context. Assuming we keep nulls, I'd like to keep
working on a keyword. But I think we could add it later.

Btw what do you think of the READ COMMITTED issues I brought up in my
third patch? We follow MariaDB here, but not DB2. DB2's behavior is
less problematic for users, although their isolation levels don't
quite match ours. If we're not okay with those results, we should
address them before merging the main patch.

Rebased to 1c33a2d81d.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-03-17T14:29:07Z

On Fri, Mar 13, 2026 at 10:06 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Thu, Mar 12, 2026 at 1:39 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> > Hi Paul,
> >
> > Review of v67-0001-Add-range_get_constructor2-to-lsyscache.patch and
> > v67-0002-Add-UPDATE-DELETE-FOR-PORTION-OF.patch:
>
> Thanks for taking another look! v68 patches attached, details below:

This needed a rebase, so here it is. Now rebased up to c9babbc881.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2026-03-25T16:05:35Z

On 13.03.26 18:06, Paul A Jungwirth wrote:
>> 3.7)
>>
>> +-- UPDATE ... RETURNING returns only the updated values (not the
>> inserted side values)
>>
>> This test looks redundant with earlier tests.  Otherwise, maybe add a
>> comment about how it's different.
> 
> I don't think a top-level RETURNING test is redundant with the CTE
> test. I expanded the comment here a bit to clarify the goal. It
> addresses your question above: Should RETURNING include the inserted
> leftovers? I don't think that makes sense:
> 
> 1. Our docs say, "The optional RETURNING clause causes UPDATE to
> compute and return value(s) based on each row actually updated." The
> leftovers were not updated.
> 
> 2. Conceptually, the leftovers represent what *didn't* change.
> 
> 3. If you implemented this with a trigger, you also wouldn't get the
> inserted leftovers.
> 
> 4. The SQL standard doesn't have RETURNING. But it does say that to
> insert the leftovers the system should execute a separate insert
> "statement". So we should do something very similar to the trigger
> case.

UPDATE ... RETURNING is in my mind equivalent to the SQL standard SELECT 
... FROM NEW TABLE (UPDATE ...) (see <data change delta table>).  So I 
was hoping for an answer there, but it just says:

"If TP simply contains a <data change delta table> DCDT, then let S be 
the <data change statement> simply contained in TP. S shall not contain 
FOR PORTION OF."

So we can pick our own behavior.  Your explanation makes sense.  (I 
suppose an alternative is that we also don't allow using FOR PORTION OF 
together with RETURNING?)

>> 5) NULL bounds
>>
>> A general comment: In particular after studying these tests in detail,
>> I'm suspicious that it's a good idea to interpret null bounds as
>> unbounded.  Expressions could return null for nested reasons, it would
>> be very hard to follow that.  Null values should mean "unknown",
>> unbounded should be explicit.  We have the keyword UNBOUNDED already,
>> maybe you could use that?  Or do you want to be able to return
>> unboundedness from an expression?
> 
> I like the idea of a keyword. I tried adding UNBOUNDED but it caused a
> few hundred S/R and R/R conflicts that I couldn't easily resolve. A
> year or two ago I had keywords here (MINVALUE/MAXVALUE IIRC), but it
> required some nasty parser hacks. This is a pretty delicate area of
> the grammar, because we have a_expr with FROM and TO and no
> punctuation. I'm already doing some contortions to handle `FOR PORTION
> OF valid_at FROM t1 + INTERVAL '1' YEAR TO MONTH TO t2`.
> 
> A keyword is not offered by the standard here, so it would just be
> custom syntactic sugar. No other RDBMS has one (I think).
> 
> I think NULL is the right choice for unbounded. It is what range types
> use, and we want this to mesh well with them. More important it works
> for *any type*. We don't always have +/-Infinity.
> 
> Also I think we should expand user choice rather than restrict it. If
> users want to forbid nulls, they can (e.g. by using a domain type).
> But if we forbid it, there is no way to override that decision.
> 
> Going back to the UNBOUNDED keyword: if we forbid nulls, then a
> keyword doesn't really add clarity, since users would already say
> `-Infinity` or `Infinity`. It's really just a way to express what null
> means in this context. Assuming we keep nulls, I'd like to keep
> working on a keyword. But I think we could add it later.

Yeah, this seems like something we could change later with relative 
ease.  Maybe solicit some input from the public during beta?

> Btw what do you think of the READ COMMITTED issues I brought up in my
> third patch? We follow MariaDB here, but not DB2. DB2's behavior is
> less problematic for users, although their isolation levels don't
> quite match ours. If we're not okay with those results, we should
> address them before merging the main patch.

It's still hard to understand.  I would be ok in general to say that 
results might be unexpected unless you use REPEATABLE READ.  Especially 
as it seems that a technical solution to improve this would be possible 
later.  But we should document this in more detail.  The verbal 
explanations are hard to interpret.  Could you maybe come up with a 
couple of ASCII-art flow charts that explains how things could go 
strange that we could put into the documentation?

Could we actually put some of these strange/unexpected behaviors into 
the isolation tests?  Right now we only test that the workaround works 
but not the initial problem.  Is this possible?  (Would we need 
injection points?)

Could we cut back the isolation tests a bit? They are the second slowest 
isolation test now, and the second largest expected file.  Maybe we 
don't need to test SERIALIZE separately?  (Assume that SERIALIZE is as 
good as or better than REPEATABLE READ?)

Attached is a patch with a few small cosmetic corrections.  In 
ExecForPortionOfLeftovers(), the comment and code that I delete is 
duplicated before and inside the loop.  The one before the
loop is probably sufficient.

Other than all that, this patch set (0001 through 0003) seems good to me.

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-03-27T21:38:32Z

Updated patches attached.

On Wed, Mar 25, 2026 at 9:05 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> > 4. The SQL standard doesn't have RETURNING. But it does say that to
> > insert the leftovers the system should execute a separate insert
> > "statement". So we should do something very similar to the trigger
> > case.
>
> UPDATE ... RETURNING is in my mind equivalent to the SQL standard SELECT
> ... FROM NEW TABLE (UPDATE ...) (see <data change delta table>).  So I
> was hoping for an answer there, but it just says:
>
> "If TP simply contains a <data change delta table> DCDT, then let S be
> the <data change statement> simply contained in TP. S shall not contain
> FOR PORTION OF."
>
> So we can pick our own behavior.  Your explanation makes sense.  (I
> suppose an alternative is that we also don't allow using FOR PORTION OF
> together with RETURNING?)

Okay, thanks! I think it's worth supporting RETURNING, and this
behavior seems like the most useful.

> > Going back to the UNBOUNDED keyword: if we forbid nulls, then a
> > keyword doesn't really add clarity, since users would already say
> > `-Infinity` or `Infinity`. It's really just a way to express what null
> > means in this context. Assuming we keep nulls, I'd like to keep
> > working on a keyword. But I think we could add it later.
>
> Yeah, this seems like something we could change later with relative
> ease.  Maybe solicit some input from the public during beta?

That sounds good to me. Also if we did want to forbid nulls here, we
could choose to do it when valid_at is a PERIOD (once that lands) but
permit them when it's a range column. That seems like it best matches
the expected behavior in both scenarios. In fact, if the PERIOD's
start/end columns are NOT NULL, then we are already enforcing the rule
indirectly.

> > Btw what do you think of the READ COMMITTED issues I brought up in my
> > third patch? We follow MariaDB here, but not DB2. DB2's behavior is
> > less problematic for users, although their isolation levels don't
> > quite match ours. If we're not okay with those results, we should
> > address them before merging the main patch.
>
> It's still hard to understand.  I would be ok in general to say that
> results might be unexpected unless you use REPEATABLE READ.  Especially
> as it seems that a technical solution to improve this would be possible
> later.  But we should document this in more detail.  The verbal
> explanations are hard to interpret.  Could you maybe come up with a
> couple of ASCII-art flow charts that explains how things could go
> strange that we could put into the documentation?

I made a sequence diagram and rewrote that section of the docs. I
think it's much clearer now.

> Could we actually put some of these strange/unexpected behaviors into
> the isolation tests?  Right now we only test that the workaround works
> but not the initial problem.  Is this possible?  (Would we need
> injection points?)

That's a good idea; I should have done it before. Done. No injection
points needed.

> Could we cut back the isolation tests a bit? They are the second slowest
> isolation test now, and the second largest expected file.  Maybe we
> don't need to test SERIALIZE separately?  (Assume that SERIALIZE is as
> good as or better than REPEATABLE READ?)

I removed all but a couple of the SERIALIZABLE tests.

> Attached is a patch with a few small cosmetic corrections.  In
> ExecForPortionOfLeftovers(), the comment and code that I delete is
> duplicated before and inside the loop.  The one before the
> loop is probably sufficient.

Applied.

> Other than all that, this patch set (0001 through 0003) seems good to me.

Thanks! These v70 patches are rebased onto f39cb8c011.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

jian he <jian.universality@gmail.com> — 2026-04-07T04:03:52Z

hi.
https://git.postgresql.org/cgit/postgresql.git/commit/?id=8e72d914c52876525a90b28444453de8085c866f

DROP TABLE If EXISTS tt;
CREATE TABLE tt(id int, valid_at int4range, amt int, CONSTRAINT
fpo2_check CHECK (upper(valid_at) <> '11'));
CREATE OR REPLACE FUNCTION dummy_update_func() RETURNS trigger AS $$
BEGIN
  RAISE NOTICE 'dummy_update_func(%) called: action = %, old = %, new = %',
    TG_ARGV[0], TG_OP, OLD, NEW;
  RETURN NEW;
END;
$$ LANGUAGE plpgsql;
CREATE TRIGGER some_trig_before BEFORE UPDATE OR INSERT ON tt FOR EACH
ROW EXECUTE PROCEDURE dummy_update_func('before');
INSERT INTO tt VALUES (1, '[1,100)', 2);
UPDATE tt FOR PORTION OF valid_at FROM 1 TO 12 SET amt = 3;

NOTICE:  dummy_update_func(before) called: action = UPDATE, old =
(1,"[1,100)",2), new = (1,"[1,12)",3)
NOTICE:  dummy_update_func(before) called: action = INSERT, old =
<NULL>, new = (1,"[12,100)",2)

As you can see, ExecGetAllUpdatedCols does not account for the valid_at column,
even though it is actively being updated. ExecGetAllUpdatedCols is being used
serval places, IMHO, we need to add some comments on
ExecGetAllUpdatedCols explaining
this behavior and maybe add some regression tests.

I'm not sure if it's safe for ExecGetAllUpdatedCols to ignore the FOR
PORTION OF column.

I reliazed this issue because of https://commitfest.postgresql.org/patch/6270/
I saw your transformForPortionOfClause comments.
        /*
         * The range column will change, but you don't need UPDATE permission
         * on it, so we don't add to updatedCols here. XXX: If
         * https://www.postgresql.org/message-id/CACJufxEtY1hdLcx%3DFhnqp-ERcV1PhbvELG5COy_CZjoEW76ZPQ%40mail.gmail.com
         * is merged (only validate CHECK constraints if they depend on one of
         * the columns being UPDATEd), we need to make sure that code knows
         * that we are updating the application-time column.
         */
But this comment is about FOR PORTION OF column permission, not about
ExecGetAllUpdatedCols.

-------------------------------------------------------------------------------------------------------------------
transformForPortionOfClause
    if (contain_volatile_functions_after_planning((Expr *) result->targetRange))
        ereport(ERROR,
                (errmsg("FOR PORTION OF bounds cannot contain volatile
functions")));

Need
errcode(ERRCODE_FEATURE_NOT_SUPPORTED).

coerce_to_target_type function comment:
 * This is the general-purpose entry point for arbitrary type coercion
 * operations.  Direct use of the component operations can_coerce_type,
 * coerce_type, and coerce_type_typmod should be restricted to special
 * cases (eg, when the conversion is expected to succeed).

We should use coerce_to_target_type more, not can_coerce_type,
coerce_type individually.
coerce_to_target_type also handles `UNKNOWN` constant, which ensures
the deparsing casts to the correct data type.

please see the attached refactoring for
https://git.postgresql.org/cgit/postgresql.git/commit/?id=8e72d914c52876525a90b28444453de8085c866f



--
jian
https://www.enterprisedb.com/

Re: SQL:2011 Application Time Update & Delete

Peter Eisentraut <peter@eisentraut.org> — 2026-04-07T11:53:13Z

On 27.03.26 22:38, Paul A Jungwirth wrote:
>> Other than all that, this patch set (0001 through 0003) seems good to me.
> Thanks! These v70 patches are rebased onto f39cb8c011.

I have committed the patches 0001 through 0003.  (I did some editing on 
the documentation.)  I think this is about as far as we can go for this 
release.

Please check the follow-up bug report(?) posted in this thread.

Re: SQL:2011 Application Time Update & Delete

SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-04-07T14:32:40Z

Hi Peter, Paul,

Please see a few bug reports related to this at [1], [2], [3].
Additionally, it appears there is another issue here:

 A BEFORE UPDATE trigger that modifies the range column creates overlapping
rows. The trigger widening the range doesn't affect leftover computation,
which uses the original FPO bounds. Result: updated row overlaps both
leftovers.

SET datestyle TO ISO, YMD;

CREATE TABLE fpo_trigger_overlap (
    id int,
    valid_at daterange,
    val text
);

-- BEFORE UPDATE trigger that resets the range to the full year
CREATE FUNCTION widen_range() RETURNS trigger AS $$
BEGIN
    NEW.valid_at := daterange('2024-01-01', '2025-01-01');
    RETURN NEW;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER trg_widen BEFORE UPDATE ON fpo_trigger_overlap
    FOR EACH ROW EXECUTE FUNCTION widen_range();

INSERT INTO fpo_trigger_overlap
    VALUES (1, '[2024-01-01, 2025-01-01)', 'original');

UPDATE fpo_trigger_overlap
    FOR PORTION OF valid_at FROM '2024-04-01' TO '2024-09-01'
    SET val = 'modified';


-- Detect overlaps (should be 0 rows for correct behavior):
SELECT a.valid_at AS range_a, a.val AS val_a,
       b.valid_at AS range_b, b.val AS val_b
FROM fpo_trigger_overlap a, fpo_trigger_overlap b
WHERE a.ctid < b.ctid AND a.valid_at && b.valid_at;

-- cleanup
DROP TABLE fpo_trigger_overlap;
DROP FUNCTION widen_range();

[1]
https://www.postgresql.org/message-id/CAHg%2BQDcd%3Dt69gLf9yQexO07EJ2mx0Z70NFHo6h94X1EDA%3DhM0g%40mail.gmail.com

[2]
https://www.postgresql.org/message-id/CAHg%2BQDcsXsUVaZ%2BJwM02yDRQEi%3DcL_rTH_ROLDYgOx004sQu7A%40mail.gmail.com

[3]
https://www.postgresql.org/message-id/CANE55rCqcse_pwXBMWhbj3_7XROb8Dks6%3DOLFmKy3bO3zDsCsg%40mail.gmail.com

Thanks,
Satya

On Tue, Apr 7, 2026 at 4:53 AM Peter Eisentraut <peter@eisentraut.org>
wrote:

> On 27.03.26 22:38, Paul A Jungwirth wrote:
> >> Other than all that, this patch set (0001 through 0003) seems good to
> me.
> > Thanks! These v70 patches are rebased onto f39cb8c011.
>
> I have committed the patches 0001 through 0003.  (I did some editing on
> the documentation.)  I think this is about as far as we can go for this
> release.
>
> Please check the follow-up bug report(?) posted in this thread.
>
>
>
>

Re: SQL:2011 Application Time Update & Delete

SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-04-09T19:35:13Z

Hi Paul, Peter,

I found a Server crash when using UPDATE ... FOR PORTION OF or DELETE ...
FOR PORTION OF on a view that has INSTEAD OF triggers.

Repro:

CREATE TABLE t (id INT, valid_at daterange, val INT);
INSERT INTO t VALUES (1, '[2026-01-01,2026-12-31)', 100);
CREATE VIEW v AS SELECT * FROM t;

CREATE FUNCTION v_trig() RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
    UPDATE t SET val = NEW.val WHERE id = OLD.id;
    RETURN NEW;
END;
$$;
CREATE TRIGGER trg INSTEAD OF UPDATE ON v
    FOR EACH ROW EXECUTE FUNCTION v_trig();

-- This crashes the server:
UPDATE v FOR PORTION OF valid_at FROM '2026-04-01' TO '2026-08-01'
    SET val = 999 WHERE id = 1;

I am thinking we should just reject this case. Attached a draft patch to
fix the issue.

Thanks,
Satya

Re: SQL:2011 Application Time Update & Delete

SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-04-09T19:42:23Z

On Thu, Apr 9, 2026 at 12:35 PM SATYANARAYANA NARLAPURAM <
satyanarlapuram@gmail.com> wrote:

> Hi Paul, Peter,
>
> I found a Server crash when using UPDATE ... FOR PORTION OF or DELETE ...
> FOR PORTION OF on a view that has INSTEAD OF triggers.
>
> Repro:
>
> CREATE TABLE t (id INT, valid_at daterange, val INT);
> INSERT INTO t VALUES (1, '[2026-01-01,2026-12-31)', 100);
> CREATE VIEW v AS SELECT * FROM t;
>
> CREATE FUNCTION v_trig() RETURNS trigger LANGUAGE plpgsql AS $$
> BEGIN
>     UPDATE t SET val = NEW.val WHERE id = OLD.id;
>     RETURN NEW;
> END;
> $$;
> CREATE TRIGGER trg INSTEAD OF UPDATE ON v
>     FOR EACH ROW EXECUTE FUNCTION v_trig();
>
> -- This crashes the server:
> UPDATE v FOR PORTION OF valid_at FROM '2026-04-01' TO '2026-08-01'
>     SET val = 999 WHERE id = 1;
>
> I am thinking we should just reject this case. Attached a draft patch to
> fix the issue.
>

Patches attached now.

Re: SQL:2011 Application Time Update & Delete

jian he <jian.universality@gmail.com> — 2026-04-14T04:33:07Z

On Fri, Apr 10, 2026 at 3:42 AM SATYANARAYANA NARLAPURAM
<satyanarlapuram@gmail.com> wrote:
>
>> Repro:
>>
>> CREATE TABLE t (id INT, valid_at daterange, val INT);
>> INSERT INTO t VALUES (1, '[2026-01-01,2026-12-31)', 100);
>> CREATE VIEW v AS SELECT * FROM t;
>>
>> CREATE FUNCTION v_trig() RETURNS trigger LANGUAGE plpgsql AS $$
>> BEGIN
>>     UPDATE t SET val = NEW.val WHERE id = OLD.id;
>>     RETURN NEW;
>> END;
>> $$;
>> CREATE TRIGGER trg INSTEAD OF UPDATE ON v
>>     FOR EACH ROW EXECUTE FUNCTION v_trig();
>>
>> -- This crashes the server:
>> UPDATE v FOR PORTION OF valid_at FROM '2026-04-01' TO '2026-08-01'
>>     SET val = 999 WHERE id = 1;
>>
>> I am thinking we should just reject this case. Attached a draft patch to fix the issue.
>
Yech, we should reject it.

In RewriteQuery, we have:
/*
 * If there was no unqualified INSTEAD rule, and the target relation
 * is a view without any INSTEAD OF triggers, see if the view can be
 * automatically updated.  If so, we perform the necessary query
 * transformation here and add the resulting query to the
 * product_queries list, so that it gets recursively rewritten if
 * necessary.  For MERGE, the view must be automatically updatable if
 * any of the merge actions lack a corresponding INSTEAD OF trigger.
 *
 * If the view cannot be automatically updated, we throw an error here
 * which is OK since the query would fail at runtime anyway.  Throwing
 * the error here is preferable to the executor check since we have
 * more detailed information available about why the view isn't
 * updatable.
 */
if (!instead &&
    rt_entry_relation->rd_rel->relkind == RELKIND_VIEW &&
    !view_has_instead_trigger(rt_entry_relation, event,
                            parsetree->mergeActionList))

Per above, RewriteQuery does not rewrite the view relation to its base
relation when the view has an INSTEAD OF trigger.
In such cases, ExecInitModifyTable->ExecInitResultRelation initialize
mtstate->resultRelInfo
using the view relation itself (rather than the underlying base table).
But ExecForPortionOfLeftovers->table_tuple_fetch_row_version requires the
relation to physical storage.

Therefore DELETE/UPDATE ... FOR PORTION OF operations cannot cope with
views that have INSTEAD OF triggers.
IMHO, rejecting it at  RewriteQuery make more sense to me.

Now the error message is:
ERROR:  UPDATE FOR PORTION OF is not supported for views with INSTEAD
OF triggers
ERROR:  DELETE FOR PORTION OF is not supported for views with INSTEAD
OF triggers



--
jian
https://www.enterprisedb.com/

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-15T05:34:11Z

On Tue, Apr 7, 2026 at 7:32 AM SATYANARAYANA NARLAPURAM
<satyanarlapuram@gmail.com> wrote:
>
> Hi Peter, Paul,
>
> Please see a few bug reports related to this at [1], [2], [3].

Thanks for collecting all these bugs together and for already working
on patches for them! I've started going through them; I'll respond to
each thread individually.

For the bug here (widening the range in a BEFORE UPDATE trigger), see below:

> Additionally, it appears there is another issue here:
>
>  A BEFORE UPDATE trigger that modifies the range column creates overlapping rows. The trigger widening the range doesn't affect leftover computation, which uses the original FPO bounds. Result: updated row overlaps both leftovers.
>
> SET datestyle TO ISO, YMD;
>
> CREATE TABLE fpo_trigger_overlap (
>     id int,
>     valid_at daterange,
>     val text
> );
>
> -- BEFORE UPDATE trigger that resets the range to the full year
> CREATE FUNCTION widen_range() RETURNS trigger AS $$
> BEGIN
>     NEW.valid_at := daterange('2024-01-01', '2025-01-01');
>     RETURN NEW;
> END;
> $$ LANGUAGE plpgsql;
>
> CREATE TRIGGER trg_widen BEFORE UPDATE ON fpo_trigger_overlap
>     FOR EACH ROW EXECUTE FUNCTION widen_range();
>
> INSERT INTO fpo_trigger_overlap
>     VALUES (1, '[2024-01-01, 2025-01-01)', 'original');
>
> UPDATE fpo_trigger_overlap
>     FOR PORTION OF valid_at FROM '2024-04-01' TO '2024-09-01'
>     SET val = 'modified';
>
>
> -- Detect overlaps (should be 0 rows for correct behavior):
> SELECT a.valid_at AS range_a, a.val AS val_a,
>        b.valid_at AS range_b, b.val AS val_b
> FROM fpo_trigger_overlap a, fpo_trigger_overlap b
> WHERE a.ctid < b.ctid AND a.valid_at && b.valid_at;
>
> -- cleanup
> DROP TABLE fpo_trigger_overlap;
> DROP FUNCTION widen_range();

I'm working on a fix for this. It's not quite ready, but I can finish
it in the morning. . . .

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-15T17:30:21Z

On Tue, Apr 14, 2026 at 10:34 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> >  A BEFORE UPDATE trigger that modifies the range column creates overlapping rows. The trigger widening the range doesn't affect leftover computation, which uses the original FPO bounds. Result: updated row overlaps both leftovers.
>
> I'm working on a fix for this. It's not quite ready, but I can finish
> it in the morning. . . .

Actually I think the proper behavior here is to raise an error. We
forbid setting the application-time column when using FOR PORTION OF
(per the standard), so why should we allow a BEFORE trigger to set it?
I think it has the same inconsistency problems. We could support it,
but then why not support both?

Assuming we want to raise an error, I think the best way is to check
the tuple in ExecForPortionOfLeftovers to see if a trigger has
modified it, and in that case raise an error. What do you think?

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-15T21:59:21Z

On Mon, Apr 6, 2026 at 9:04 PM jian he <jian.universality@gmail.com> wrote:
>
> As you can see, ExecGetAllUpdatedCols does not account for the valid_at column,
> even though it is actively being updated. ExecGetAllUpdatedCols is being used
> serval places, IMHO, we need to add some comments on
> ExecGetAllUpdatedCols explaining
> this behavior and maybe add some regression tests.
>
> I'm not sure if it's safe for ExecGetAllUpdatedCols to ignore the FOR
> PORTION OF column.

The other threads have found a couple problems with that now. I wonder
if we should have ExecGetExtraUpdatedCols add the application-time
attno to the returned bitmapset? Or even add it to updatedCols in
analysis and then ignore it for permission checking. That seems more
robust than finding all the places we need to add it, except
updatedCols is in a struct called RTEPermissionInfo. Best of all I
think would be to add a new bitmapset somewhere else and not use
permissions infrastructure for GENERATED columns, UPDATE OF triggers,
skipping CHECK constraints, etc. But is it too late in the cycle to
make a change like that?

In the short term, what about just doing this?:

@@ -1449,6 +1449,7 @@ ExecGetAllUpdatedCols(ResultRelInfo *relinfo,
EState *estate)
     oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));

     ret = bms_union(ExecGetUpdatedCols(relinfo, estate),
+                    ExecGetForPortionOfCol(relinfo, estate),
                     ExecGetExtraUpdatedCols(relinfo, estate));

     MemoryContextSwitchTo(oldcxt);

(Implementing that function is left as an exercise for the reader.)

> transformForPortionOfClause
>     if (contain_volatile_functions_after_planning((Expr *) result->targetRange))
>         ereport(ERROR,
>                 (errmsg("FOR PORTION OF bounds cannot contain volatile
> functions")));
>
> Need
> errcode(ERRCODE_FEATURE_NOT_SUPPORTED).

Okay.

> coerce_to_target_type function comment:
>  * This is the general-purpose entry point for arbitrary type coercion
>  * operations.  Direct use of the component operations can_coerce_type,
>  * coerce_type, and coerce_type_typmod should be restricted to special
>  * cases (eg, when the conversion is expected to succeed).
>
> We should use coerce_to_target_type more, not can_coerce_type,
> coerce_type individually.
> coerce_to_target_type also handles `UNKNOWN` constant, which ensures
> the deparsing casts to the correct data type.

Including the casts when we deparse does seem like an improvement.

The patch looks good to me.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-15T23:25:58Z

On Mon, Apr 13, 2026 at 9:33 PM jian he <jian.universality@gmail.com> wrote:
>
> On Fri, Apr 10, 2026 at 3:42 AM SATYANARAYANA NARLAPURAM
> <satyanarlapuram@gmail.com> wrote:
> >
> >> Repro:
> >>
> >> CREATE TABLE t (id INT, valid_at daterange, val INT);
> >> INSERT INTO t VALUES (1, '[2026-01-01,2026-12-31)', 100);
> >> CREATE VIEW v AS SELECT * FROM t;
> >>
> >> CREATE FUNCTION v_trig() RETURNS trigger LANGUAGE plpgsql AS $$
> >> BEGIN
> >>     UPDATE t SET val = NEW.val WHERE id = OLD.id;
> >>     RETURN NEW;
> >> END;
> >> $$;
> >> CREATE TRIGGER trg INSTEAD OF UPDATE ON v
> >>     FOR EACH ROW EXECUTE FUNCTION v_trig();
> >>
> >> -- This crashes the server:
> >> UPDATE v FOR PORTION OF valid_at FROM '2026-04-01' TO '2026-08-01'
> >>     SET val = 999 WHERE id = 1;
> >>
> >> I am thinking we should just reject this case. Attached a draft patch to fix the issue.
> >
> Yech, we should reject it.

I think using INSTEAD OF triggers to replace an UPDATE/DELETE FOR
PORTION OF is a valid use-case, but it doesn't make sense to insert
temporal leftovers. As you say, we can't access the underlying
storage. But also we don't know what changes the trigger actually
made. The trigger should be responsible for leftovers, and we
shouldn't try to add more. So I think the fix is just to skip
inserting leftovers. I've attached a patch to do that.

This is a good use-case for a pending followup patch (which will have
to wait for v20 I think), which makes the FOR PORTION OF parameters
accessible to triggers. We need that ourselves for PERIOD foreign keys
with CASCADE/SET NULL/SET DEFAULT, but it's nice to have another
example of why you might want it.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-18T23:18:13Z

On Wed, Apr 15, 2026 at 10:30 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Tue, Apr 14, 2026 at 10:34 PM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > >  A BEFORE UPDATE trigger that modifies the range column creates overlapping rows. The trigger widening the range doesn't affect leftover computation, which uses the original FPO bounds. Result: updated row overlaps both leftovers.
> >
> > I'm working on a fix for this. It's not quite ready, but I can finish
> > it in the morning. . . .
>
> Actually I think the proper behavior here is to raise an error. We
> forbid setting the application-time column when using FOR PORTION OF
> (per the standard), so why should we allow a BEFORE trigger to set it?
> I think it has the same inconsistency problems. We could support it,
> but then why not support both?
>
> Assuming we want to raise an error, I think the best way is to check
> the tuple in ExecForPortionOfLeftovers to see if a trigger has
> modified it, and in that case raise an error. What do you think?

Here is a patch that forbids changing the valid_at column in a BEFORE
trigger. It works by capturing the value before triggers run, then
checking afterwards if it is still the same (using the default btree
equality operator; probably a simple binary comparison is good
enough).

This copy+check only happens if the table has BEFORE UPDATE row
triggers, so there is no cost in most cases.

I'm raising ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION, which is what we
use when (basically) a trigger & UPDATE both change a row in a way
that leaves the user intent unclear. I think that's a very close fit
here, but you could argue we should use the same errcode as SETing
valid_at. That is ERRCODE_SYNTAX_ERROR. That strikes me as a
questionable choice, actually. Personally I think using different
errcodes is correct though.

In ExecForPortionOfSaveRange there is a lot of code duplication
copying the structure for child partitions, but I think we could cut
that by first adding jian he's helper function (ExecInitForPortionOf)
from another bugfix patch [1].

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

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-19T18:10:02Z

Peter Eisentraut <peter@eisentraut.org> writes:
> I have committed the patches 0001 through 0003.

Coverity is complaining that rsi.isDone may be used uninitialized in
ExecForPortionOfLeftovers.  It's correct: that function is not obeying
the function call protocol, and it's only accidental that it's not
failing.  In ValuePerCall mode the caller is supposed to initialize
isDone (and isnull too) before each call.  The canonical reference
for this is execSRF.c, and it does that.  So I think we need something
like the attached.

I notice that execSRF.c also runs pgstat_init_function_usage and
pgstat_end_function_usage around each call.  That's not too important
right now, but I wonder whether we should add it while we're looking
at this.  It would perhaps be important once we support user-defined
withoutPortionProcs.

			regards, tom lane

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-19T18:51:23Z

On Sun, Apr 19, 2026 at 11:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter@eisentraut.org> writes:
> > I have committed the patches 0001 through 0003.
>
> Coverity is complaining that rsi.isDone may be used uninitialized in
> ExecForPortionOfLeftovers.  It's correct: that function is not obeying
> the function call protocol, and it's only accidental that it's not
> failing.  In ValuePerCall mode the caller is supposed to initialize
> isDone (and isnull too) before each call.  The canonical reference
> for this is execSRF.c, and it does that.  So I think we need something
> like the attached.

Thanks for the patch! Your changes look good to me.

> I notice that execSRF.c also runs pgstat_init_function_usage and
> pgstat_end_function_usage around each call.  That's not too important
> right now, but I wonder whether we should add it while we're looking
> at this.  It would perhaps be important once we support user-defined
> withoutPortionProcs.

I agree we should do that. Here is a patch with that added to your changes.

I was curious why execSRF.c uses `rsinfo.isDone != ExprMultipleResult`
for the finalize parameter, because I don't think a SRF should ever
return ExprSingleResult, right? So I guess it is just to be cautious.
Makes sense. I followed that approach.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-20T14:33:54Z

Paul A Jungwirth <pj@illuminatedcomputing.com> writes:
> On Sun, Apr 19, 2026 at 11:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I notice that execSRF.c also runs pgstat_init_function_usage and
>> pgstat_end_function_usage around each call.  That's not too important
>> right now, but I wonder whether we should add it while we're looking
>> at this.  It would perhaps be important once we support user-defined
>> withoutPortionProcs.

> I agree we should do that. Here is a patch with that added to your changes.

Pushed, thanks.

> I was curious why execSRF.c uses `rsinfo.isDone != ExprMultipleResult`
> for the finalize parameter, because I don't think a SRF should ever
> return ExprSingleResult, right? So I guess it is just to be cautious.
> Makes sense. I followed that approach.

It's been awhile, but I think these specs were set with the intention
that if a plain function were somehow called as a SRF, it would act as
though it were a SRF returning one row.  We haven't quite reached that
with this patch --- I think it'd be an infinite loop as
ExecForPortionOfLeftovers() stands.  I'm content with the way things
are though, given that it should always be the case that special
privileges are needed to mark a function as being a
withoutPortionProcs function.

But speaking of infinite loops, should this one contain a
CHECK_FOR_INTERRUPTS call?  It's hard to conceive of a case where
the value would be broken down finely enough for that to be a
problem, but ...

			regards, tom lane

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-20T15:48:31Z

On Mon, Apr 20, 2026 at 7:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > I was curious why execSRF.c uses `rsinfo.isDone != ExprMultipleResult`
> > for the finalize parameter, because I don't think a SRF should ever
> > return ExprSingleResult, right? So I guess it is just to be cautious.
> > Makes sense. I followed that approach.
>
> It's been awhile, but I think these specs were set with the intention
> that if a plain function were somehow called as a SRF, it would act as
> though it were a SRF returning one row.  We haven't quite reached that
> with this patch --- I think it'd be an infinite loop as
> ExecForPortionOfLeftovers() stands.  I'm content with the way things
> are though, given that it should always be the case that special
> privileges are needed to mark a function as being a
> withoutPortionProcs function.
>
> But speaking of infinite loops, should this one contain a
> CHECK_FOR_INTERRUPTS call?  It's hard to conceive of a case where
> the value would be broken down finely enough for that to be a
> problem, but ...

A rangetype could only loop 0-2 times; a multirange 0-1. So I don't
think we need it. Eventually user-defined types could loop more, but a
design that inserts many records every time you change something seems
like a bad idea. Maybe I would add it anyway just out of caution, but
I suspect it's excessive.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-20T16:03:30Z

Paul A Jungwirth <pj@illuminatedcomputing.com> writes:
> On Mon, Apr 20, 2026 at 7:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> But speaking of infinite loops, should this one contain a
>> CHECK_FOR_INTERRUPTS call?  It's hard to conceive of a case where
>> the value would be broken down finely enough for that to be a
>> problem, but ...

> A rangetype could only loop 0-2 times; a multirange 0-1. So I don't
> think we need it. Eventually user-defined types could loop more, but a
> design that inserts many records every time you change something seems
> like a bad idea. Maybe I would add it anyway just out of caution, but
> I suspect it's excessive.

Fair enough.  It's quite likely that we'd hit at least one CFI down
inside the insertion anyway.

			regards, tom lane

Re: SQL:2011 Application Time Update & Delete

jian he <jian.universality@gmail.com> — 2026-04-21T06:25:06Z

On Thu, Apr 16, 2026 at 7:26 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> I think using INSTEAD OF triggers to replace an UPDATE/DELETE FOR
> PORTION OF is a valid use-case, but it doesn't make sense to insert
> temporal leftovers. As you say, we can't access the underlying
> storage. But also we don't know what changes the trigger actually
> made. The trigger should be responsible for leftovers, and we
> shouldn't try to add more. So I think the fix is just to skip
> inserting leftovers. I've attached a patch to do that.
>
hi.

CREATE TABLE fpo_instead_base (id int, valid_at daterange, val int);
INSERT INTO fpo_instead_base VALUES (1, '[2024-01-01,2024-12-31)', 100);
CREATE VIEW fpo_instead_view AS SELECT * FROM fpo_instead_base;
CREATE FUNCTION fpo_instead_trig_fn() RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
    RETURN NEW;
END;
$$;
CREATE TRIGGER fpo_instead_trig INSTEAD OF UPDATE ON fpo_instead_view
  FOR EACH ROW EXECUTE FUNCTION fpo_instead_trig_fn();

UPDATE fpo_instead_view FOR PORTION OF valid_at FROM '2024-04-01' TO
'2024-08-01'
    SET val = 999 WHERE id = 1
    RETURNING *;

 id |        valid_at         | val
----+-------------------------+-----
  1 | [2024-01-01,2024-12-31) | 999
(1 row)

Should I expect the column `valid_at` value as [2024-04-01,2024-08-01) ?

We should also document this on doc/src/sgml/ref/update.sgml
Attached is a minor regession test enhancement for
"v2-0001-Fix-INSTEAD-OF-triggers-with-DELETE-UPDATE-FOR-PO.patch".



--
jian
https://www.enterprisedb.com/

Re: SQL:2011 Application Time Update & Delete

jian he <jian.universality@gmail.com> — 2026-04-21T09:51:18Z

On Sun, Apr 19, 2026 at 7:18 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Here is a patch that forbids changing the valid_at column in a BEFORE
> trigger. It works by capturing the value before triggers run, then
> checking afterwards if it is still the same (using the default btree
> equality operator; probably a simple binary comparison is good
> enough).
>
> This copy+check only happens if the table has BEFORE UPDATE row
> triggers, so there is no cost in most cases.
>
> I'm raising ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION, which is what we
> use when (basically) a trigger & UPDATE both change a row in a way
> that leaves the user intent unclear. I think that's a very close fit
> here, but you could argue we should use the same errcode as SETing
> valid_at. That is ERRCODE_SYNTAX_ERROR. That strikes me as a
> questionable choice, actually. Personally I think using different
> errcodes is correct though.
>
HI.
After applying v1-0001-Forbid-BEFORE-UPDATE-triggers-changing-the-FOR-PO.patch

----------------------------------------------------
CREATE OR REPLACE FUNCTION trg_fponum() RETURNS TRIGGER LANGUAGE plpgsql AS
$$
BEGIN
  NEW.valid_at = '[1,12)';
  raise notice 'old: %, new: %', old, new;
  RETURN NEW;
END;
$$;
create table fpo3(valid_at int4range, b int);
CREATE TRIGGER fpo_before_update_row BEFORE UPDATE ON fpo3 FOR EACH
ROW EXECUTE PROCEDURE trg_fponum();
insert into fpo3 values('[1,100]', 1);
UPDATE fpo3 FOR PORTION OF valid_at FROM 1 TO 12 SET b = 2;
----------------------------------------------------
The above works as expected, but the below is not what i expected.

create type textrange as range (subtype = text, collation = "C");
CREATE OR REPLACE FUNCTION trg_fpo()
RETURNS TRIGGER LANGUAGE plpgsql AS
$$
BEGIN
  NEW.valid_at = '[A,d)';
  raise notice 'old: %, new: %', old, new;
  RETURN NEW;
END;
$$;

create table fpo1(valid_at textrange, b int);
CREATE TRIGGER fpo_before_update_row BEFORE UPDATE ON fpo1 FOR EACH
ROW EXECUTE PROCEDURE trg_fpo();
insert into fpo1 values ('[a,d]', 1);

UPDATE fpo1 FOR PORTION OF valid_at FROM 'A' TO 'd' SET b = 2;
NOTICE:  old: ("[a,d]",1), new: ("[A,d)",2)
ERROR:  cannot change column "valid_at" from a BEFORE trigger because
it is used in FOR PORTION OF

Should I expect this to work without error, just like the table fpo3
UPDATE FOR PORTION OF statement above?



--
jian
https://www.enterprisedb.com/

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-22T19:50:27Z

On Mon, Apr 20, 2026 at 11:25 PM jian he <jian.universality@gmail.com> wrote:
>
> On Thu, Apr 16, 2026 at 7:26 AM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > I think using INSTEAD OF triggers to replace an UPDATE/DELETE FOR
> > PORTION OF is a valid use-case, but it doesn't make sense to insert
> > temporal leftovers. As you say, we can't access the underlying
> > storage. But also we don't know what changes the trigger actually
> > made. The trigger should be responsible for leftovers, and we
> > shouldn't try to add more. So I think the fix is just to skip
> > inserting leftovers. I've attached a patch to do that.
> >
> hi.
>
> CREATE TABLE fpo_instead_base (id int, valid_at daterange, val int);
> INSERT INTO fpo_instead_base VALUES (1, '[2024-01-01,2024-12-31)', 100);
> CREATE VIEW fpo_instead_view AS SELECT * FROM fpo_instead_base;
> CREATE FUNCTION fpo_instead_trig_fn() RETURNS trigger LANGUAGE plpgsql AS $$
> BEGIN
>     RETURN NEW;
> END;
> $$;
> CREATE TRIGGER fpo_instead_trig INSTEAD OF UPDATE ON fpo_instead_view
>   FOR EACH ROW EXECUTE FUNCTION fpo_instead_trig_fn();
>
> UPDATE fpo_instead_view FOR PORTION OF valid_at FROM '2024-04-01' TO
> '2024-08-01'
>     SET val = 999 WHERE id = 1
>     RETURNING *;
>
>  id |        valid_at         | val
> ----+-------------------------+-----
>   1 | [2024-01-01,2024-12-31) | 999
> (1 row)
>
> Should I expect the column `valid_at` value as [2024-04-01,2024-08-01) ?

Yes, because we ran an INSTEAD OF trigger and skipped the UPDATE
(including setting the start/end dates).

> We should also document this on doc/src/sgml/ref/update.sgml
> Attached is a minor regession test enhancement for
> "v2-0001-Fix-INSTEAD-OF-triggers-with-DELETE-UPDATE-FOR-PO.patch".

Thanks! I squashed those patches and did some minor cleanup. I posted
v4 to this dedicated thread:
https://www.postgresql.org/message-id/CA%2BrenyVenLk%2Bu%3DyGvDAyeFEuvkmeQx448-KnnGczqQHB10_fbg%40mail.gmail.com

I also made a commitfest entry pointing there. Let's continue on that
thread so that future messages & patches get tracked correctly (and
not as part of the original feature's CF entry).

Hmm I forgot to add the documentation first. So I'll do that and post
a v5 shortly.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-06-18T02:29:17Z

On Tue, Apr 21, 2026 at 2:51 AM jian he <jian.universality@gmail.com> wrote:
>
> On Sun, Apr 19, 2026 at 7:18 AM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > Here is a patch that forbids changing the valid_at column in a BEFORE
> > trigger. It works by capturing the value before triggers run, then
> > checking afterwards if it is still the same (using the default btree
> > equality operator; probably a simple binary comparison is good
> > enough).
> >
> > ...
> The above works as expected, but the below is not what i expected.
>
> create type textrange as range (subtype = text, collation = "C");
> CREATE OR REPLACE FUNCTION trg_fpo()
> RETURNS TRIGGER LANGUAGE plpgsql AS
> $$
> BEGIN
>   NEW.valid_at = '[A,d)';
>   raise notice 'old: %, new: %', old, new;
>   RETURN NEW;
> END;
> $$;
>
> create table fpo1(valid_at textrange, b int);
> CREATE TRIGGER fpo_before_update_row BEFORE UPDATE ON fpo1 FOR EACH
> ROW EXECUTE PROCEDURE trg_fpo();
> insert into fpo1 values ('[a,d]', 1);
>
> UPDATE fpo1 FOR PORTION OF valid_at FROM 'A' TO 'd' SET b = 2;
> NOTICE:  old: ("[a,d]",1), new: ("[A,d)",2)
> ERROR:  cannot change column "valid_at" from a BEFORE trigger because
> it is used in FOR PORTION OF
>
> Should I expect this to work without error, just like the table fpo3
> UPDATE FOR PORTION OF statement above?

That looks correct to me. In the C collation, uppercase letters come
before lowercase.
The row started as '[a,d)'. Then FOR PORTION OF valid_at FROM 'A' to
'd' leaves it as '[a,d)'
(because the intersection can only narrow). Then your BEFORE trigger
changes it to '[A,d)'.

Here is a new patch though, rebased and improved in a couple ways.

First, we are able to use ExecInitForPortionOf, added by another fix.
This reduces a lot of code duplication.

Also I moved the check out of ExecForPortionOfLeftovers. Now the time
between capturing the pre-trigger value and checking it is shorter: we
do the check right after triggers fire. This also means we don't have
to add fields to ForPortionOfState.

Yours,

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

Re: SQL:2011 Application Time Update & Delete

Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-07-02T21:39:57Z

On Wed, Jun 17, 2026 at 7:29 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> > On Sun, Apr 19, 2026 at 7:18 AM Paul A Jungwirth
> > <pj@illuminatedcomputing.com> wrote:
> > >
> > > Here is a patch that forbids changing the valid_at column in a BEFORE
> > > trigger. It works by capturing the value before triggers run, then
> > > checking afterwards if it is still the same (using the default btree
> > > equality operator; probably a simple binary comparison is good
> > > enough).
>
> Here is a new patch though, rebased and improved in a couple ways.
>
> First, we are able to use ExecInitForPortionOf, added by another fix.
> This reduces a lot of code duplication.
>
> Also I moved the check out of ExecForPortionOfLeftovers. Now the time
> between capturing the pre-trigger value and checking it is shorter: we
> do the check right after triggers fire. This also means we don't have
> to add fields to ForPortionOfState.

I did some research on what MariaDB and DB2 do here.

MariaDB allows changing the before/end values in a BEFORE trigger. It
gives the same results as Postgres today. I tested with:

CREATE TABLE t (id int, ds date, de date, name text, period for
valid_at (ds, de));
DELIMITER //
CREATE TRIGGER t_before_update_row before UPDATE ON t FOR EACH ROW
BEGIN
  SET NEW.ds = '2001-01-01';
END; //
DELIMITER ;
insert into t values (1, '2020-01-01', '2030-01-01', 'hi');
update t for portion of valid_at from '2021-06-01' to '2021-01-01' set
ds = '2001-01-01'; -- fails
update t for portion of valid_at from '2020-06-01' to '2021-01-01' set
name = 'two'; -- okay
select * from t;

The result was:

+------+------------+------------+------+
| id   | ds         | de         | name |
+------+------------+------------+------+
|    1 | 2001-01-01 | 2021-01-01 | two  |
|    1 | 2020-01-01 | 2020-06-01 | hi   |
|    1 | 2021-01-01 | 2030-01-01 | hi   |
+------+------------+------------+------+

Note that if you add a temporal primary key, truncate, and try again,
the previously-allowed command will fail, because the PK blocks the
duplicate.

In DB2, the change is not allowed. I tested against the Community
Edition for Docker and got an error. I couldn't even define the
trigger! And the docs at
https://www.ibm.com/docs/en/db2/11.5.x?topic=statements-create-trigger
say:

> BUSINESS_TIME period columns: The start and end columns of a BUSINESS_TIME period cannot be changed in the body of BEFORE UPDATE trigger (SQLSTATE 42808).

We can't really do what DB2 does, because we don't have PERIODs (yet).
So we have to wait 'til the UPDATE FOR PORTION OF to detect the
problem. For us, the error is at runtime, not at trigger definition
time.

I'm okay with either behavior. I haven't found a rule in the standard.
I feel that DB2 is more correct, but I also think the user is trying
to do something weird here, and if they get weird results it is okay.
If they have a primary key or unique constraint, it still blocks
duplicates. If we don't want to implement the check from my previous
patch, that is okay with me. Here is a v3 alternative which just adds
tests showing what Postgres does.

Yours,

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