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