Re: SQL:2011 application time

On 01.03.24 22:56, Paul Jungwirth wrote:
> On 3/1/24 12:38, Paul Jungwirth wrote:
>> On 2/29/24 13:16, Paul Jungwirth wrote:
>> Here is a v26 patch series to fix a cfbot failure in sepgsql. Rebased
>> to 655dc31046.
>
> v27 attached, fixing some cfbot failures from
> headerscheck+cpluspluscheck. Sorry for the noise!

I had committed v27-0001-Rename-conwithoutoverlaps-to-conperiod.patch a
little while ago.

I have reviewed v27-0002 through 0004 now. I have one semantic question
below, and there are a few places where more clarification of the
interfaces could help. Other than that, I think this is pretty good.

Attached is a small patch that changes the PERIOD keyword to unreserved
for this patch. You had said earlier that this didn't work for you.
The attached patch works for me when applied on top of 0003.

* v27-0002-Add-GiST-referencedagg-support-func.patch

You wrote:

> I'm not sure how else to do it. The issue is that `range_agg` returns
> a multirange, so the result
> type doesn't match the inputs. But other types will likely have the
> same problem: to combine boxes
> you may need a multibox. The combine mdranges you may need a
> multimdrange.

Can we just hardcode the use of range_agg for this release? Might be
easier. I don't see all this generality being useful in the near future.

> Btw that part changed a bit since v24 because as jian he pointed out,
> our type system doesn't
> support anyrange inputs and an anyrange[] output. So I changed the
> support funcs to use SETOF.

I didn't see any SETOF stuff in the patch, or I didn't know where to look.

I'm not sure I follow all the details here. So more explanations of any
kind could be helpful.

* v27-0003-Refactor-FK-operator-lookup.patch

I suggest to skip this refactoring patch. I don't think the way this is
sliced up is all that great, and it doesn't actually help with the
subsequent patches.

* v27-0004-Add-temporal-FOREIGN-KEYs.patch

- src/backend/catalog/pg_constraint.c

FindFKPeriodOpersAndProcs() could use a bit more top-level
documentation. Where does the input opclass come from? What are the
three output values? What is the business with "symmetric types"?

- src/backend/commands/indexcmds.c

GetOperatorFromWellKnownStrategy() is apparently changed to accept
InvalidOid for rhstype, but the meaning of this is not explained in
the function header. It's also not clear to me why an existing caller
is changed. This should be explained more thoroughly.

- src/backend/commands/tablecmds.c

is_temporal and similar should be renamed to with_period or similar
throughout this patch.

In transformFkeyGetPrimaryKey():

* Now build the list of PK attributes from the indkey definition (we
- * assume a primary key cannot have expressional elements)
+ * assume a primary key cannot have expressional elements, unless it
+ * has a PERIOD)

I think the original statement is still true even with PERIOD. The
expressional elements refer to expression indexes. I don't think we can
have a PERIOD marker on an expression?

- src/backend/utils/adt/ri_triggers.c

Please remove the separate trigger functions for the period case. They
are the same as the non-period ones, so we don't need separate ones.
The difference is handled lower in the call stack, which I think is a
good setup. Removing the separate functions also removes a lot of extra
code in other parts of the patch.

- src/include/catalog/pg_constraint.h

Should also update catalogs.sgml accordingly.

- src/test/regress/expected/without_overlaps.out
- src/test/regress/sql/without_overlaps.sql

A few general comments on the tests:

- In the INSERT commands, specify the column names explicitly. This
makes the tests easier to read (especially since the column order
between the PK and the FK table is sometimes different).

- Let's try to make it so that the inserted literals match the values
shown in the various error messages, so it's easier to match them up.
So, change the int4range literals to half-open notation. And also maybe
change the date output format to ISO.

- In various comments, instead of test FK "child", maybe use
"referencing table"? Instead of "parent", use "referenced table" (or
primary key table). When I read child and parent I was looking for
inheritance.

- Consider truncating the test tables before each major block of tests
and refilling them with fresh data. So it's easier to eyeball the
tests. Otherwise, there is too much dependency on what earlier tests
left behind.

A specific question:

In this test, a PERIOD marker on the referenced site is automatically
inferred from the primary key:

+-- with inferred PK on the referenced table:
+CREATE TABLE temporal_fk_rng2rng (
+ id int4range,
+ valid_at tsrange,
+ parent_id int4range,
+ CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT
OVERLAPS),
+ CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD
valid_at)
+ REFERENCES temporal_rng
+);

In your patch, this succeeds. According to the SQL standard, it should
not. In subclause 11.8, syntax rule 4b:

"""
Otherwise, the table descriptor of the referenced table shall include a
unique constraint UC that specifies PRIMARY KEY. The table constraint
descriptor of UC shall not include an application time period name.
"""

So this case is apparently explicitly ruled out.

(It might be ok to make an extension here, but then we should be
explicit about it.)