Re: SQL:2011 application time

On 9/17/23 20:11, jian he wrote:
> small issues so far I found, v14.

Thank you again for the review! v15 is attached.

> IndexInfo struct definition comment still has Temporal related
> comment, should be removed.

Fixed.

> catalog-pg-index.html, no indperiod doc entry, also in table pg_index,
> column indperiod  is junk value now.

You're right, it is just unneeded now that PERIODs are implemented by 
GENERATED columns. I've removed it.

> I think in UpdateIndexRelation, you need an add indperiod to build a
> pg_index tuple, similar to what you did in CreateConstraintEntry.

It's gone now.

> seems to make the following query works, we need to bring btree_gist
> related code to core?
> CREATE TABLE temporal_fk_rng2rng22 (id int8, valid_at int4range, > unique (id, valid_at WITHOUT OVERLAPS));

It doesn't need to be brought into core, but you would need to say 
`CREATE EXTENSION btree_gist` first. Since the regression tests don't 
assume we've built contrib, we have to use a workaround there.

> /* ----------------
>   * pg_period definition.  cpp turns this into
>   * typedef struct FormData_pg_period
>   * ----------------
>   */
> CATALOG(pg_period,8000,PeriodRelationId)
> {
> Oid oid; /* OID of the period */
> NameData pername; /* name of period */
> Oid perrelid; /* OID of relation containing this period */
> int16 perstart; /* column for start value */
> int16 perend; /* column for end value */
> int16 perrange; /* column for range value */
> Oid perconstraint; /* OID of (start < end) constraint */
> } FormData_pg_period;
> 
> no idea what the above comment "cpp'' refers to.

I believe cpp = C Pre-Processor. This comment is at the top of all the 
catalog/pg_*.h files. The next line is part of the same sentence (which 
took me a while to notice :-).

> The sixth field in
> FormData_pg_period: perrange, the comment conflict with catalogs.sgml
>>> perrngtype oid (references pg_type.oid)
>>> The OID of the range type associated with this period

You're right, fixed! More cruft from the old PERIOD implementation.

> create table pt (id integer, ds date, de date, period for p (ds, de));
> SELECT table_name, column_name, column_default, is_nullable,
> is_generated, generation_expression
> FROM    information_schema.columns
> WHERE table_name = 'pt' ORDER BY 1, 2;
> 
> the hidden generated column  (p)  is_nullable return NO. but ds, de
> is_nullable both return YES. so column p is_nullable should return
> YES?

The is_nullable behavior is correct I believe. In a range if the 
lower/upper value is NULL, it signifies the range has no lower/upper 
bound. So it's fine for ds or de to be NULL, but not the range itself (p).

Technically the SQL spec says that the PERIOD start & end columns should 
be NOT NULL, but that forces people to use ugly sentinel values like 
'3999-01-01'. It's a shame to make people do that when NULL works so 
well instead. Our time-related types do have Infinity and -Infinity 
which is not as ugly, but many other types do not. Plus those values 
interact badly with ranges. For example `select '(,)'::daterange - 
'(,Infinity)'::daterange` gives the infinitesimal result `[infinity,)`. 
I've heard at least one report of that make a mess in a user's database. 
If a user wants to make the start/end columns NOT NULL they can, so I 
prefer not to force them.

Continuing to your other email:

On 9/18/23 05:49, jian he wrote:
 > BEGIN;
 > ...
 > ALTER TABLE temporal_fk_rng2rng ALTER CONSTRAINT
 > temporal_fk_rng2rng_fk  DEFERRABLE INITIALLY DEFERRED;
 >
 > delete from temporal_rng; ---should not fail.
 > commit; ---fail in here.

Great catch! This is fixed also.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com