Re: SQL:2011 Application Time Update & Delete

> On Nov 13, 2025, at 12:07, Paul A Jungwirth <pj(at)illuminatedcomputing(dot)com> wrote:
>
> I'll reply to Chao Li separately, but those changes are included in
> the patches here.
>
> Rebased to 705601c5ae.
>
> Yours,
>
> --
> Paul ~{:-)
> pj(at)illuminatedcomputing(dot)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/