Re: SQL:2011 application time

Hello,

Here are new patches consolidating feedback from several emails.
I haven't addressed everything but I think I'm overdue for a reply:

On 1/4/24 21:06, jian he wrote:
>
> I am confused.
> say condition: " (cmp_l1l2 <= 0 && cmp_u1l2 >= 0 && cmp_u1u2 <= 0)"
> the following code will only run PartA, never run PartB?
>
> `
> else if (cmp_l1l2 >= 0 && cmp_u1u2 <= 0)
> PartA
> else if (cmp_l1l2 <= 0 && cmp_u1l2 >= 0 && cmp_u1u2 <= 0)
> PartB
> `
>
> minimum example:
> #include<stdio.h>
> #include<string.h>
> #include<stdlib.h>
> #include<assert.h>
> int
> main(void)
> {
> int cmp_l1l2;
> int cmp_u1u2;
> int cmp_u1l2;
> int cmp_l1u2;
> cmp_l1u2 = -1;
> cmp_l1l2 = 0;
> cmp_u1u2 = 0;
> cmp_u1l2 = 0;
> assert(cmp_u1l2 == 0);
> if (cmp_l1u2 > 0 || cmp_u1l2 < 0)
> printf("calling partA\n");
> else if (cmp_l1l2 >= 0 && cmp_u1u2 <= 0)
> printf("calling partB\n");
> else if (cmp_l1l2 <= 0 && cmp_u1l2 >= 0 && cmp_u1u2 <= 0)
> printf("calling partC\n");
> }

All of the branches are used. I've attached a `without_portion.c` minimal example showing different
cases. For ranges it helps to go through the Allen relationships
(https://en.wikipedia.org/wiki/Allen%27s_interval_algebra) to make a comprehensive check. (But note
that our operators don't exactly match that terminology, and it's important to consider
closed-vs-open and unbounded cases.)

> I am confused with the name "range_without_portion", I think
> "range_not_overlap" would be better.

I think I covered this in my other reply and we are now in agreement, but if that's mistaken let
know me.

> select numrange(1.1, 2.2) @- numrange(2.0, 3.0);
> the result is not the same as
> select numrange(2.0, 3.0) @- numrange(1.1, 2.2);

Correct, @- is not commutative.

> So your categorize oprkind as 'b' for operator "@-" is wrong?
> select oprname,oprkind,oprcanhash,oprcanmerge,oprleft,oprright,oprresult,oprcode
> from pg_operator
> where oprname = '@-';

'b' is the correct oprkind. It is a binary (infix) operator.

> aslo
> select count(*), oprkind from pg_operator group by oprkind;
> there are only 5% are prefix operators.
> maybe we should design it as:
> 1. if both inputs are empty range, the result array is empty.
> 2. if both inputs are non-empty and never overlaps, put both of them
> to the result array.
> 3. if one input is empty another one is not, then put the non-empty
> one into the result array.

Also covered before, but if any of this still applies please let me know.

> after applying the patch: now the catalog data seems not correct to me.
> SELECT a1.amopfamily
> ,a1.amoplefttype::regtype
> ,a1.amoprighttype
> ,a1.amopstrategy
> ,amoppurpose
> ,amopsortfamily
> ,amopopr
> ,op.oprname
> ,am.amname
> FROM pg_amop as a1 join pg_operator op on op.oid = a1.amopopr
> join pg_am am on am.oid = a1.amopmethod
> where amoppurpose = 'p';
> output:
> amopfamily | amoplefttype | amoprighttype | amopstrategy |
> amoppurpose | amopsortfamily | amopopr | oprname | amname
>
------------+---------------+---------------+--------------+-------------+----------------+---------+---------+--------
> 2593 | box | 603 | 31 | p
> | 0 | 803 | # | gist
> 3919 | anyrange | 3831 | 31 | p
> | 0 | 3900 | * | gist
> 6158 | anymultirange | 4537 | 31 | p
> | 0 | 4394 | * | gist
> 3919 | anyrange | 3831 | 32 | p
> | 0 | 8747 | @- | gist
> 6158 | anymultirange | 4537 | 32 | p
> | 0 | 8407 | @- | gist
> (5 rows)
>
> select oprcode, oprname, oprleft::regtype
> from pg_operator opr
> where opr.oprname in ('#','*','@-')
> and oprleft = oprright
> and oprleft in (603,3831,4537);
> output:
>
> oprcode | oprname | oprleft
> ----------------------------+---------+---------------
> box_intersect | # | box
> range_intersect | * | anyrange
> multirange_intersect | * | anymultirange
> range_without_portion | @- | anyrange
> multirange_without_portion | @- | anymultirange
> (5 rows)

This seems correct. '#' is the name of the box overlaps operator. Probably I should add a box @-
operator too. But see below. . . .

> should amoppurpose = 'p' is true apply to ' @-' operator?

Yes.

> catalog-pg-amop.html:
> `
> amopsortfamily oid (references pg_opfamily.oid):
> The B-tree operator family this entry sorts according to, if an
> ordering operator; zero if a search operator
> `
> you should also update the above entry, the amopsortfamily is also
> zero for "portion operator" for the newly implemented "portion
> operator".

Okay, done.

> v21-0006-Add-UPDATE-DELETE-FOR-PORTION-OF.patch
> create mode 100644 src/backend/utils/adt/period.c
> create mode 100644 src/include/utils/period.h
> you should put these two files to v21-0008-Add-PERIODs.patch.
> it's not related to that patch, it also makes people easy to review.

You're right, sorry!

On 1/8/24 16:00, jian he wrote:
>
> +/*
> + * 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)

Agreed, done.

> + 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".

Okay, I can see that. I used "rangeSet" because we add it to the SET clause of the UPDATE command.
Here I've changed it to rangeTargetList. I think this matches other code and better indicates what
it holds. Any objections?

In the PERIOD patch we will need two TLEs here (that's why it's a List): one for the start column
and one for the end column.

> 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");

I think you're right. According to the comments on TM_Result (returned by table_tuple_update), a
TM_Ok indicates that the lock was acquired.

> +/* ----------------------------------------------------------------
> + * 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."

Changed to "untouched portion".

> + 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.

I added the first assert. The second is not true for non-range types.

> + <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.

Still TODO.

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

Fixed.

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

Still TODO.

On 1/8/24 21:33, jian he wrote:
>
> src5=# select range_without_portion(numrange(1.0,3.0,'[]'),
> numrange(1.5,2.0,'(]'));
> range_without_portion
> ---------------------------
> {"[1.0,1.5]","(2.0,3.0]"}
> (1 row)
>
> src5=# \gdesc
> Column | Type
> -----------------------+-----------
> range_without_portion | numeric[]
> (1 row)
>
> src5=# \df range_without_portion
> List of functions
> Schema | Name | Result data type | Argument data
> types | Type
> ------------+-----------------------+------------------+---------------------+------
> pg_catalog | range_without_portion | anyarray | anyrange,
> anyrange | func
> (1 row)
>
> so apparently, you cannot from (anyrange, anyrange) get anyarray the
> element type is anyrange.
> I cannot find the documented explanation in
> https://www.postgresql.org/docs/current/extend-type-system.html#EXTEND-TYPES-POLYMORPHIC
>
> anyrange is POLYMORPHIC, anyarray is POLYMORPHIC,
> but I suppose, getting an anyarray the element type is anyrange would be hard.

You're right, that is a problem.

I think the right approach is to make intersect and without_portion just be support functions, not
operators. Then I don't need to introduce the new 'p' amop strategy at all, which seemed like a
dubious idea anyway. Then the without_portion function can return a SETOF instead of an array.

Another idea is to add more polymorphic types, anyrangearray and anymultirangearray, but maybe that
is too big a thing. OTOH I have wanted those same types before. I will take a stab at it.

On 1/11/24 06:44, Peter Eisentraut wrote:
> Here is some more detailed review of the first two patches. (I reviewed v20; I see you have also
> posted v21, but they don't appear very different for this purpose.)
>
> v20-0001-Add-stratnum-GiST-support-function.patch
>
> * contrib/btree_gist/Makefile
>
> Needs corresponding meson.build updates.

Fixed.

> * contrib/btree_gist/btree_gist--1.7--1.8.sql
>
> Should gist_stratnum_btree() live in contrib/btree_gist/ or in core?
> Are there other extensions that use the btree strategy numbers for
> gist?

Moved. None of our other contrib extensions use it. I thought it would be friendly to offer it to
outside extensions, but maybe that is too speculative.

> +ALTER OPERATOR FAMILY gist_vbit_ops USING gist ADD
> + FUNCTION 12 (varbit, varbit) gist_stratnum_btree (int2) ;
>
> Is there a reason for the extra space after FUNCTION here (repeated
> throughout the file)?

Fixed.

> +-- added in 1.4:
>
> What is the purpose of these "added in" comments?

I added those to help me make sure I was including every type in the extension, but I've taken them
out here.

> v20-0002-Add-temporal-PRIMARY-KEY-and-UNIQUE-constraints.patch
>
> * contrib/btree_gist/Makefile
>
> Also update meson.build.

Done.

> * contrib/btree_gist/sql/without_overlaps.sql
>
> Maybe also insert a few values, to verify that the constraint actually
> does something?

Done.

> * doc/src/sgml/ref/create_table.sgml
>
> Is "must have a range type" still true? With the changes to the
> strategy number mapping, any type with a supported operator class
> should work?

Updated. Probably more docs to come; I want to go through them all now that we support more types.

> * src/backend/utils/adt/ruleutils.c
>
> Is it actually useful to add an argument to
> decompile_column_index_array()? Wouldn't it be easier to just print
> the " WITHOUT OVERLAPS" in the caller after returning from it?

Okay, done.

> * src/include/access/gist_private.h
>
> The added function gistTranslateStratnum() isn't really "private" to
> gist. So access/gist.h would be a better place for it.

Moved.

> Also, most other functions there appear to be named "GistSomething",
> so a more consistent name might be GistTranslateStratnum.
>
> * src/include/access/stratnum.h

Changed.

> The added StrategyIsValid() doesn't seem that useful? Plenty of
> existing code just compares against InvalidStrategy, and there is only
> one caller for the new function. I suggest to do without it.
>
> * src/include/commands/defrem.h

Okay, removed.

> We are using two terms here, well-known strategy number and canonical
> strategy number, to mean the same thing (I think?). Let's try to
> stick with one. Or explain the relationship?

True. Changed everything to "well-known" which seems like a better match for what's going on.

I haven't gone through jian he's Jan 13 patch yet, but since he was also implementing Peter's
requests I thought I should share what I have. I did this work a while ago, but I was hoping to
finish the TODOs above first, and then we got hit with a winter storm that knocked out power. Sorry
to cause duplicate work!

Rebased to 2f35c14cfb.

Yours,

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com