SQL:2011 Application Time Update & Delete

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

On Sun, Jun 22, 2025 at 6:19 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

Here are updated patches for UPDATE/DELETE FOR PORTION OF and related
functionality. I left out the usual PERIODs patch because I'm still
updating it to work with the latest master.

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

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

Yours,

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

On Fri, Aug 29, 2025 at 6:03 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

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

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

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

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

Rebased to d96c854dfc.

Yours,

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

On Wed, Sep 24, 2025 at 9:05 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

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

More rebase & CI fixes attached.

Rebased to 03d40e4b52 now.

Yours,

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

On Sat, Oct 4, 2025 at 12:48 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

On Wed, Sep 24, 2025 at 9:05 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

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

More rebase & CI fixes attached.

Rebased to 03d40e4b52 now.

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

Now rebased to 7a662a46eb.

Yours,

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

On Sun, Oct 12, 2025 at 11:43 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

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

More rebase & CI fixes attached.

Rebased to 03d40e4b52 now.

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

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

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

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

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

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

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

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

Rebased to c0677d8b2e.

Yours,

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

On 24.10.25 19:08, Paul A Jungwirth wrote:

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

I have looked through the documentation patches 0001 through 0003.

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

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

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

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

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

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

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

On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 24.10.25 19:08, Paul A Jungwirth wrote:

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

I have looked through the documentation patches 0001 through 0003.

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

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

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

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

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

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

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

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

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

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

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

I agree it is better that way.

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

Okay, changed to numeric and removed the dollar signs.

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

Thanks!

Yours,

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

On Wed, Oct 29, 2025 at 11:02 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 24.10.25 19:08, Paul A Jungwirth wrote:

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

I have looked through the documentation patches 0001 through 0003.

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

Hi Hackers,

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

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

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

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

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

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

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

Rebased to 65f4976189.

Yours,

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

On 30.10.25 07:02, Paul A Jungwirth wrote:

On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 24.10.25 19:08, Paul A Jungwirth wrote:

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

I have looked through the documentation patches 0001 through 0003.

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

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

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

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

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

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

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

On Tue, Nov 4, 2025 at 11:12 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

Back to Postgres, you can get "desired" results IN READ COMMITTED by
explicitly locking rows (with SELECT FOR UPDATE) just before
updating/deleting them. Since you acquire the lock before the
update/delete starts, there can be no new leftovers created within
that span of history, and the update/delete sees everything that is
there.

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

Yours,

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

I have looked at the patch

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

This seems sound in principle.

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

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

A few other minor details:

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

+#include "utils/array.h"

seems to be unused.

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

This could be written just as a struct, like

struct range_minus_multi_fctx
{
...
};

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

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

* src/include/catalog/pg_proc.dat

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

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

On Nov 5, 2025, at 03:12, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:

On Wed, Oct 29, 2025 at 11:02 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 24.10.25 19:08, Paul A Jungwirth wrote:

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

I have looked through the documentation patches 0001 through 0003.

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

Hi Hackers,

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

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

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

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

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

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

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

Rebased to 65f4976189.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com
<v59-0003-Document-temporal-update-delete.patch><v59-0005-Add-UPDATE-DELETE-FOR-PORTION-OF.patch><v59-0001-Add-docs-section-for-temporal-tables-with-primar.patch><v59-0004-Add-range_minus_multi-and-multirange_minus_multi.patch><v59-0007-Add-tg_temporal-to-TriggerData.patch><v59-0002-Document-temporal-foreign-keys.patch><v59-0008-Look-up-more-temporal-foreign-key-helper-procs.patch><v59-0009-Add-CASCADE-SET-NULL-SET-DEFAULT-for-temporal-fo.patch><v59-0006-Add-isolation-tests-for-UPDATE-DELETE-FOR-PORTIO.patch><v59-0011-Add-PERIODs.patch><v59-0010-Expose-FOR-PORTION-OF-to-plpgsql-triggers.patch>

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

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

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

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

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

I will continue to review the rest of commits tomorrow.

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

On Nov 12, 2025, at 17:31, Chao Li <li.evan.chao@gmail.com> wrote:

On Nov 5, 2025, at 03:12, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:

On Wed, Oct 29, 2025 at 11:02 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

On Tue, Oct 28, 2025 at 3:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 24.10.25 19:08, Paul A Jungwirth wrote:

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

I have looked through the documentation patches 0001 through 0003.

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

Hi Hackers,

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

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

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

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

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

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

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

Rebased to 65f4976189.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com
<v59-0003-Document-temporal-update-delete.patch><v59-0005-Add-UPDATE-DELETE-FOR-PORTION-OF.patch><v59-0001-Add-docs-section-for-temporal-tables-with-primar.patch><v59-0004-Add-range_minus_multi-and-multirange_minus_multi.patch><v59-0007-Add-tg_temporal-to-TriggerData.patch><v59-0002-Document-temporal-foreign-keys.patch><v59-0008-Look-up-more-temporal-foreign-key-helper-procs.patch><v59-0009-Add-CASCADE-SET-NULL-SET-DEFAULT-for-temporal-fo.patch><v59-0006-Add-isolation-tests-for-UPDATE-DELETE-FOR-PORTIO.patch><v59-0011-Add-PERIODs.patch><v59-0010-Expose-FOR-PORTION-OF-to-plpgsql-triggers.patch>

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

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

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

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

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

I will continue to review the rest of commits tomorrow.

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

A few more comments:

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

The common before ) is not needed.

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

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

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

Typo: where -> were

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

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

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

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

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 Tue, Nov 11, 2025 at 11:42 PM Peter Eisentraut <peter@eisentraut.org> wrote:

I have looked at the patch

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

This seems sound in principle.

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

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

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

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

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

but not this:

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

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

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

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

A few other minor details:

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

+#include "utils/array.h"

seems to be unused.

You're right; removed.

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

This could be written just as a struct, like

struct range_minus_multi_fctx
{
...
};

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

Okay.

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

Done, with some extra word-smithing.

* src/include/catalog/pg_proc.dat

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

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

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

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

Rebased to 705601c5ae.

Yours,

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

On Nov 13, 2025, at 12:07, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:

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

Rebased to 705601c5ae.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com
<v60-0001-Fix-typo-in-documentation-about-application-time.patch><v60-0004-Add-UPDATE-DELETE-FOR-PORTION-OF.patch><v60-0003-Add-range_minus_multi-and-multirange_minus_multi.patch><v60-0002-Document-temporal-update-delete.patch><v60-0005-Add-isolation-tests-for-UPDATE-DELETE-FOR-PORTIO.patch><v60-0006-Add-tg_temporal-to-TriggerData.patch><v60-0009-Expose-FOR-PORTION-OF-to-plpgsql-triggers.patch><v60-0007-Look-up-more-temporal-foreign-key-helper-procs.patch><v60-0008-Add-CASCADE-SET-NULL-SET-DEFAULT-for-temporal-fo.patch><v60-0010-Add-PERIODs.patch>

I continue reviewing ...

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

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

I will number the comments continuously.

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

This is a little confusing, “FOR PORTION OF” is not a command, it’s just a clause inside UDDATE or DELETE. So maybe 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/

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/