Re: SQL:2011 application time - Mailing list pgsql-hackers

Thank you for continuing to review this submission! My changes are in
the v18 patch I sent a few days ago. Details below.

On Sun, Oct 29, 2023 at 5:01 PM jian he <jian.universality@gmail.com> wrote:
> * The attached patch makes foreign keys with PERIOD fail if any of the
> foreign key columns is "generated columns".

I don't see anything like that included in your attachment. I do see
the restriction on `ON DELETE SET NULL/DEFAULT (columnlist)`, which I
included. But you are referring to something else I take it? Why do
you think FKs should fail if the referred column is GENERATED? Is that
a restriction you think should apply to all FKs or only temporal ones?

> * The following queries will cause segmentation fault. not sure the
> best way to fix it.
> . . .
> CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD valid_at)
> REFERENCES temporal3 (id, valid_at)
> );

Fixed, with additional tests re PERIOD on one side but not the other.

> * change the function FindFKComparisonOperators's "eqstrategy"  to
> make pg_constraint record correct {conpfeqop,conppeqop,conffeqop}.

This change is incorrect because it causes foreign keys to fail when
created with btree_gist. See my reply to Peter for more about that. My
v18 patch also includes some new (very simple) tests in the btree_gist
extension so it's easier to see whether temporal PKs & FKs work there.

> * fix the ON DELETE SET NULL/DEFAULT (columnlist). Now the following
> queries error will be more consistent.
> ALTER TABLE temporal_fk_rng2rng DROP CONSTRAINT temporal_fk_rng2rng_fk,
> ADD CONSTRAINT temporal_fk_rng2rng_fk
> FOREIGN KEY (parent_id, PERIOD valid_at) REFERENCES temporal_rng
> ON DELETE SET DEFAULT(valid_at);
> --ON DELETE SET NULL(valid_at);

Okay, thanks!

> * refactor restrict_cascading_range function.

It looks like your attachment only renames the column, but I think
"restrict" is more expressive and accurate than "get", so I'd like to
keep the original name here.

> * you did if (numfks != numpks) before if (is_temporal) {numfks +=
> 1;}, So I changed the code order to make the error report more
> consistent.

Since we do numfks +=1 and numpks +=1, I don't see any inconsistency
here. Also you are making things now happen before a permissions
check, which may be important (I'm not sure). Can you explain what
improvement is intended here? Your changes don't seem to cause any
changes in the tests, so what is the goal? Perhaps I'm
misunderstanding what you mean by "more consistent."

Thanks! I'll reply to your Nov 6 email separately.

Yours,

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