Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: SQL:2011 Application Time Update & Delete
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CA+renyU-iz_zvM0gGP=dvBPVrz=Jj3qdCjtAh5nLZRhb49xMFw@mail.gmail.com>
Date: Fri, 14 Nov 2025 12:10:00 +0800
Cc: Peter Eisentraut <peter@eisentraut.org>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <6F8D7105-BD1C-432D-84F3-BC688C0C8EDC@gmail.com>
References: <ec498c3d-5f2b-48ec-b989-5561c8aa2024@illuminatedcomputing.com>
 <CA+renyW-S0LyG0E4qxFvnKNKsgq_6WWeTStOXHpjCvwj6LKS6Q@mail.gmail.com>
 <CA+renyXXJJCmgG0Wdf89JgVNAeRKkVn+EuZGTf4Ph-BWoJafQA@mail.gmail.com>
 <CA+renyWA-b00qvz4gDdPbPBjVatacB1T5v7SJc6J_xg3R6-qRw@mail.gmail.com>
 <CA+renyVYnV9rGDiUhTQEy8r6gx6Xg-+OEo-2DK7JbuBXcLVq5A@mail.gmail.com>
 <CA+renyWcNBdnaW4zc9S03aN+fEbVVB1S+q8e9MEjhM2YB+kkiw@mail.gmail.com>
 <CA+renyUiEu2yTHk+Vy-Rt2xA5Vnp2N_ohN=npiNmMKTh53qDfA@mail.gmail.com>
 <2f5364f3-a1d3-4410-98f3-d788b11e6525@eisentraut.org>
 <CA+renyW7ZB_k9AgmSFJU2EegL9r1k1sgWo4-9tGGkgwxNqe6kw@mail.gmail.com>
 <CA+renyUodzxAvMnpa_LTvo+Ru1ZKH+Su8KaPvD4iMtguFKzq4g@mail.gmail.com>
 <1ace7bc1-9dd4-42c9-a473-517cef37cce9@eisentraut.org>
 <CA+renyU-iz_zvM0gGP=dvBPVrz=Jj3qdCjtAh5nLZRhb49xMFw@mail.gmail.com>
To: Paul A Jungwirth <pj@illuminatedcomputing.com>
Archived-At: 
 <https://www.postgresql.org/message-id/6F8D7105-BD1C-432D-84F3-BC688C0C8EDC%40gmail.com>
Precedence: bulk


> On Nov 13, 2025, at 12:07, Paul A Jungwirth =
<pj@illuminatedcomputing.com> wrote:
>=20
> I'll reply to Chao Li separately, but those changes are included in
> the patches here.
>=20
> Rebased to 705601c5ae.
>=20
> Yours,
>=20
> --
> 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-an=
d-multirange_minus_multi.patch><v60-0002-Document-temporal-update-delete.p=
atch><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-hel=
per-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, =E2=80=9Cgit am=E2=80=9D 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, =E2=80=9CFOR PORTION OF=E2=80=9D is not a =
command, it=E2=80=99s just a clause inside UDDATE or DELETE. So maybe =
change to:

For an <command>UPDATE/DELETE ... FOR PORTION OF<command> clause =E2=80=A6=


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

=E2=80=9CYour delete=E2=80=9D sounds not formal doc style. I searched =
over all docs and didn=E2=80=99t 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
```

=E2=80=9CYour update=E2=80=9D, 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
```

=E2=80=9CUpdate the existing row=E2=80=9D, here I think =E2=80=9Can=E2=80=9D=
 is better than =E2=80=9Cthe=E2=80=9D, because we are not referring to =
any specific row.
Then, =E2=80=9Cthere interval=E2=80=9D should be =E2=80=9Cits =
interval=E2=80=9D.

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

=E2=80=9CUn-updated=E2=80=9D sounds strange, I never saw that. Maybe =
=E2=80=9Cunchanged=E2=80=9D?

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

I don=E2=80=99t fully get this. Say, original range is 2-5:

* if update 1-6, then no insert;=20
* 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=E2=80=9D);
```

I have a question and don=E2=80=99t 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=E2=80=9D);
```

Nit: it=E2=80=99s unusual to start elog with a capital letter, so =
=E2=80=9CGot=E2=80=9D -> =E2=80=9Cgot=E2=80=9D.

15 - 0004 - nodeModifyTable.c
```
+		if (rel->rd_rel->relkind =3D=3D =
RELKIND_PARTITIONED_TABLE &&
+			mtstate->mt_partition_tuple_routing =3D=3D 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 =3D=3D =
NULL)
+			{
```

The outer =E2=80=9Cif=E2=80=9D has checked =
mtstate->mt_partition_tuple_routing =3D=3D NULL, so the inner =E2=80=9Cif=E2=
=80=9D is a redundant.

16 - 0004 - nodeFuncs.c
```
+		case T_ForPortionOfExpr:
+			{
+				ForPortionOfExpr *forPortionOf =3D =
(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=E2=80=99d 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
```

=E2=80=9CCause unexpected results=E2=80=9D sounds not formal doc style, =
suggesting =E2=80=9Cmay yield results that differ from what the user =
intends=E2=80=9D.

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

Looks like no need to assign NULL to trigdata.tg_temporal, because =
=E2=80=9Ctrigdata=E2=80=9D has bee zero-ed when defining it. In other =
places of this patch, you don=E2=80=99t 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 =3D InvalidOid;
 	Oid			opcintype =3D InvalidOid;
@@ -1693,6 +1700,17 @@ FindFKPeriodOpers(Oid opclass,
 							   =
aggedcontainedbyoperoid,
 							   &strat);
=20
+	/*
+	 * 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 =3D get_opcode(*intersectoperoid);
 }
```

I don=E2=80=99t 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 =3D estate->trigdata->tg_temporal;
+
+			if (estate->trigdata =3D=3D NULL)
+				elog(ERROR, "trigger promise is not in a =
trigger function");
```

You deference estate->trigdata before the NULL check. So the =E2=80=9Cfpo=E2=
=80=9D 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: =E2=80=9Cto=E2=80=9D is not needed.

Wow, 0010 is too big, I have spent the entire morning, so I=E2=80=99d =
leave 0010 to next week.

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