Re: SQL:2011 application time

Thanks for the thorough review and testing!

Here is a v14 patch with the segfault and incorrect handling of NO 
ACTION and RESTRICT fixed (and reproductions added to the test suite).

A few more comments below on feedback from you and Peter:

On 9/12/23 02:01, jian he wrote:
> hi. some trivial issue:
> 
> in src/backend/catalog/index.c
> /* * System attributes are never null, so no need to check. */
> if (attnum <= 0)
> 
> since you already checked attnum == 0
> so here you can just attnum < 0?

I fixed the "/* *" typo here. I'm reluctant to change the attnum 
comparison since that's not a line I touched. (It was just part of the 
context around the updated comment.) Your suggestion does make sense 
though, so perhaps it should be a separate commit?

> ERROR:  column "valid_at" named in WITHOUT OVERLAPS is not a range type
> 
> IMHO, "named" is unnecessary.

Changed.

> doc/src/sgml/catalogs.sgml
> pg_constraint adds another attribute (column): contemporal, seems no doc entry.

Added.

> also the temporal in oxford definition is "relating to time", here we
> can deal with range.
> So maybe  "temporal" is not that accurate?

I agree if we allow multiple WITHOUT OVERLAPS/etc clauses, we should 
change the terminology. I'll include that with the multiple-range-keys 
change discussed upthread.

On 9/1/23 02:30, Peter Eisentraut wrote:
 > * There is a lot of talk about "temporal" in this patch, but this
 > functionality is more general than temporal.  I would prefer to change
 > this to more neutral terms like "overlaps".

Okay, sounds like several of us agree on this.

 > * The field ii_Temporal in IndexInfo doesn't seem necessary and could
 > be  handled via local variables.  See [0] for a similar discussion:
 >
 > [0]:
 > 
https://www.postgresql.org/message-id/flat/f84640e3-00d3-5abd-3f41-e6a19d33c40b@eisentraut.org

Done.

 > * In gram.y, change withoutOverlapsClause -> without_overlaps_clause
 > for consistency with the surrounding code.

Done.

 > * No-op assignments like n->without_overlaps = NULL; can be omitted.
 > (Or you should put them everywhere.  But only in some places seems
 > inconsistent and confusing.)

Changed. That makes sense since newNode uses palloc0fast. FWIW there is 
quite a lot of other code in gram.y that sets NULL fields though, 
including in ConstraintElem, and it seems like it does improve the 
clarity a little. By "everywhere" I think you mean wherever the file 
calls makeNode(Constraint)? I might go back and do it that way later.

I'll keep working on a patch to support multiple range keys, but I 
wanted to work through the rest of the feedback first. Also there is 
some fixing to do with partitions I believe, and then I'll finish the 
PERIOD support. So this v14 patch is just some minor fixes & tweaks from 
September feedback.

Yours,

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