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