Re: SQL:2011 application time - Mailing list pgsql-hackers

On Sat, Jan 6, 2024 at 8:20 AM Paul Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Getting caught up on reviews from November and December:
>
>
> New patches attached, rebased to 43b46aae12. I'll work on your feedback from Jan 4 next. Thanks!
>

+/*
+ * ForPortionOfClause
+ * representation of FOR PORTION OF <period-name> FROM <ts> TO <te>
+ * or FOR PORTION OF <period-name> (<target>)
+ */
+typedef struct ForPortionOfClause
+{
+ NodeTag type;
+ char   *range_name;
+ int range_name_location;
+ Node   *target;
+ Node   *target_start;
+ Node   *target_end;
+} ForPortionOfClause;

"range_name_location" can be just "location"?
generally most of the struct put the "location" to the last field in the struct.
(that's the pattern I found all over other code)

+ if (isUpdate)
+ {
+ /*
+ * Now make sure we update the start/end time of the record.
+ * For a range col (r) this is `r = r * targetRange`.
+ */
+ Expr *rangeSetExpr;
+ TargetEntry *tle;
+
+ strat = RTIntersectStrategyNumber;
+ GetOperatorFromCanonicalStrategy(opclass, InvalidOid, "intersects",
"FOR PORTION OF", &opid, &strat);
+ rangeSetExpr = (Expr *) makeSimpleA_Expr(AEXPR_OP, get_opname(opid),
+ (Node *) copyObject(rangeVar), targetExpr,
+ forPortionOf->range_name_location);
+ rangeSetExpr = (Expr *) transformExpr(pstate, (Node *) rangeSetExpr,
EXPR_KIND_UPDATE_PORTION);
+
+ /* Make a TLE to set the range column */
+ result->rangeSet = NIL;
+ tle = makeTargetEntry(rangeSetExpr, range_attno, range_name, false);
+ result->rangeSet = lappend(result->rangeSet, tle);
+
+ /* Mark the range column as requiring update permissions */
+ target_perminfo->updatedCols = bms_add_member(target_perminfo->updatedCols,
+  range_attno - FirstLowInvalidHeapAttributeNumber);
+ }
+ else
+ result->rangeSet = NIL;
I think the name "rangeSet" is misleading, since "set" is generally
related to a set of records.
but here it's more about the "range intersect".

in ExecDelete
we have following code pattern:
ExecDeleteEpilogue(context, resultRelInfo, tupleid, oldtuple, changingPart);
if (processReturning && resultRelInfo->ri_projectReturning)
{
....
if (!table_tuple_fetch_row_version(resultRelationDesc, tupleid,
   SnapshotAny, slot))
elog(ERROR, "failed to fetch deleted tuple for DELETE RETURNING");
}
}

but the ExecForPortionOfLeftovers is inside ExecDeleteEpilogue.
meaning even without ExecForPortionOfLeftovers, we can still call
table_tuple_fetch_row_version
also if it was *not* concurrently updated, then our current process
holds the lock until the ending of the transaction, i think.
So the following TODO is unnecessary?

+ /*
+ * Get the range of the old pre-UPDATE/DELETE tuple,
+ * so we can intersect it with the FOR PORTION OF target
+ * and see if there are any "leftovers" to insert.
+ *
+ * We have already locked the tuple in ExecUpdate/ExecDelete
+ * (TODO: if it was *not* concurrently updated, does
table_tuple_update lock the tuple itself?
+ * I don't found the code for that yet, and maybe it depends on the AM?)
+ * and it has passed EvalPlanQual.
+ * Make sure we're looking at the most recent version.
+ * Otherwise concurrent updates of the same tuple in READ COMMITTED
+ * could insert conflicting "leftovers".
+ */
+ if (!table_tuple_fetch_row_version(resultRelInfo->ri_RelationDesc,
tupleid, SnapshotAny, oldtupleSlot))
+ elog(ERROR, "failed to fetch tuple for FOR PORTION OF");
+

+/* ----------------------------------------------------------------
+ * ExecForPortionOfLeftovers
+ *
+ * Insert tuples for the untouched timestamp of a row in a FOR
+ * PORTION OF UPDATE/DELETE
+ * ----------------------------------------------------------------
+ */
+static void
+ExecForPortionOfLeftovers(ModifyTableContext *context,
+   EState *estate,
+   ResultRelInfo *resultRelInfo,
+   ItemPointer tupleid)

maybe change the comment to
"Insert tuples for the not intersection of a row in a FOR PORTION OF
UPDATE/DELETE."

+ deconstruct_array(DatumGetArrayTypeP(allLeftovers),
typcache->type_id, typcache->typlen,
+   typcache->typbyval, typcache->typalign, &leftovers, NULL, &nleftovers);
+
+ if (nleftovers > 0)
+ {
I think add something like assert nleftovers >=0 && nleftovers <= 2
(assume only range not multirange) would improve readability.


+  <para>
+   If the table has a range column or
+   <link linkend="ddl-periods-application-periods"><literal>PERIOD</literal></link>,
+   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 span
+   extends outside the <literal>FOR PORTION OF</literal> bounds, then
your delete
+   will only change the span within those bounds. In effect you are
deleting any
+   moment targeted by <literal>FOR PORTION OF</literal> and no moments outside.
+  </para>
+
+  <para>
+   Specifically, after <productname>PostgreSQL</productname> deletes
the existing row,
+   it will <literal>INSERT</literal>
+   new rows whose range or start/end column(s) receive the remaining
span outside
+   the targeted bounds, containing the original values in other columns.
+   There will be zero to two inserted records,
+   depending on whether the original span extended before the targeted
+   <literal>FROM</literal>, after the targeted <literal>TO</literal>,
both, or neither.
+  </para>
+
+  <para>
+   These secondary inserts fire <literal>INSERT</literal> triggers. First
+   <literal>BEFORE DELETE</literal> triggers first, then
+   <literal>BEFORE INSERT</literal>, then <literal>AFTER INSERT</literal>,
+   then <literal>AFTER DELETE</literal>.
+  </para>
+
+  <para>
+   These secondary inserts do not require <literal>INSERT</literal>
privilege on the table.
+   This is because conceptually no new information has been added.
The inserted rows only preserve
+   existing data about the untargeted time period. Note this may
result in users firing <literal>INSERT</literal>
+   triggers who don't have insert privileges, so be careful about
<literal>SECURITY DEFINER</literal> trigger functions!
+  </para>

I think you need to wrap them into a big paragraph, otherwise they
lose the context?
please see the attached build sql-update.html.

also I think
+   <link linkend="ddl-periods-application-periods"><literal>PERIOD</literal></link>,
should shove into Add-PERIODs.patch.

otherwise you cannot build  Add-UPDATE-DELETE-FOR-PORTION-OF.patch
without all the patches.
I think the "FOR-PORTION-OF" feature is kind of independ?
Because, IMHO, "for portion" is a range datum interacting with another
single range datum, but the primary key with  "WITHOUT OVERLAPS", is
range datum interacting with a set of range datums.
now I cannot  just git apply v22-0006-Add-UPDATE-DELETE-FOR-PORTION-OF.patch.
That maybe would make it more difficult to get commited?