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(at)eisentraut(dot)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(at)illuminatedcomputing(dot)com