Re: SQL:2011 Application Time Update & Delete - Mailing list pgsql-hackers

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
changeto: 
>
> 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
otherplaces 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
thinkquote_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