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