Re: SQL:2011 application time

On Sat, Jan 6, 2024 at 8:20 AM Paul Jungwirth
<pj(at)illuminatedcomputing(dot)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?