Re: SQL:2011 application time

[Jeff Davis]

On Mon, 2024-05-13 at 12:11 +0200, Peter Eisentraut wrote:
> Some of these issues might be design flaws in the underlying
> mechanisms, 
> like range types and exclusion constraints.  Like, if you're supposed
> to 
> use this for scheduling but you can use empty ranges to bypass
> exclusion 
> constraints, how is one supposed to use this?

An empty range does not "bypass" the an exclusion constraint. The
exclusion constraint has a documented meaning and it's enforced.

Of course there are situations where an empty range doesn't make a lot
of sense. For many domains zero doesn't make any sense, either.
Consider receiving an email saying "thank you for purchasing 0
widgets!". Check constraints seem like a reasonable way to prevent
those kinds of problems.

Regards,
Jeff Davis

Re: SQL:2011 application time

[Peter Eisentraut]

On 15.05.24 11:39, Peter Eisentraut wrote:
Attached are the individual revert patches.  I'm supplying these here 
mainly so that future efforts can use those instead of the original 
patches, since that would have to redo all the conflict resolution and 
also miss various typo fixes etc. that were applied in the meantime.  I 
will commit this as one squashed patch.


This has been done.

Re: SQL:2011 application time

[Michael Paquier]

On Tue, May 14, 2024 at 01:33:46PM +0800, jian he wrote:
> thanks for the idea, I roughly played around with it, seems doable.
> but the timing seems not good, reverting is a good idea.

Please note that this is still an open item, and that time is running
short until beta1.  A revert seems to be the consensus reached, so,
Peter, are you planning to do so?
--
Michael


signature.asc
Description: PGP signature

Re: SQL:2011 application time

[jian he]

On Tue, May 14, 2024 at 7:30 AM Paul Jungwirth
 wrote:
>
> On 5/13/24 03:11, Peter Eisentraut wrote:
> > It looks like we missed some of these fundamental design questions early 
> > on, and it might be too
> > late now to fix them for PG17.
> >
> > For example, the discussion on unique constraints misses that the question 
> > of null values in unique
> > constraints itself is controversial and that there is now a way to change 
> > the behavior.  So I
> > imagine there is also a selection of possible behaviors you might want for 
> > empty ranges.
> > Intuitively, I don't think empty ranges are sensible for temporal unique 
> > constraints.  But anyway,
> > it's a bit late now to be discussing this.
> >
> > I'm also concerned that if ranges have this fundamental incompatibility 
> > with periods, then the plan
> > to eventually evolve this patch set to support standard periods will also 
> > have as-yet-unknown problems.
> >
> > Some of these issues might be design flaws in the underlying mechanisms, 
> > like range types and
> > exclusion constraints.  Like, if you're supposed to use this for scheduling 
> > but you can use empty
> > ranges to bypass exclusion constraints, how is one supposed to use this?  
> > Yes, a check constraint
> > using isempty() might be the right answer.  But I don't see this documented 
> > anywhere.
> >
> > On the technical side, adding an implicit check constraint as part of a 
> > primary key constraint is
> > quite a difficult implementation task, as I think you are discovering.  I'm 
> > just reminded about how
> > the patch for catalogued not-null constraints struggled with linking these 
> > not-null constraints to
> > primary keys correctly.  This sounds a bit similar.
> >
> > I'm afraid that these issues cannot be resolved in good time for this 
> > release, so we should revert
> > this patch set for now.
>
> I think reverting is a good idea. I'm not really happy with the CHECK 
> constraint solution either.
> I'd be happy to have some more time to rework this for v18.
>
> A couple alternatives I'd like to explore:
>
> 1. Domain constraints instead of a CHECK constraint. I think this is probably 
> worse, and I don't
> plan to spend much time on it, but I thought I'd mention it in case someone 
> else thought otherwise.
>
> 2. A slightly different overlaps operator, say &&&, where 'empty' &&& 'empty' 
> is true. But 'empty'
> with anything else could still be false (or not). That operator would prevent 
> duplicates in an
> exclusion constraint. This also means we could support more types than just 
> ranges & multiranges. I
> need to think about whether this combines badly with existing operators, but 
> if not it has a lot of
> promise. If anything it might be *less* contradictory, because it fits better 
> with 'empty' @>
> 'empty', which we say is true.
>
thanks for the idea, I roughly played around with it, seems doable.
but the timing seems not good, reverting is a good idea.


I also checked the commit. 6db4598fcb82a87a683c4572707e522504830a2b
+
+/*
+ * Returns the btree number for equals, otherwise invalid.
+ */
+Datum
+gist_stratnum_btree(PG_FUNCTION_ARGS)
+{
+ StrategyNumber strat = PG_GETARG_UINT16(0);
+
+ switch (strat)
+ {
+ case RTEqualStrategyNumber:
+ PG_RETURN_UINT16(BTEqualStrategyNumber);
+ case RTLessStrategyNumber:
+ PG_RETURN_UINT16(BTLessStrategyNumber);
+ case RTLessEqualStrategyNumber:
+ PG_RETURN_UINT16(BTLessEqualStrategyNumber);
+ case RTGreaterStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterStrategyNumber);
+ case RTGreaterEqualStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterEqualStrategyNumber);
+ default:
+ PG_RETURN_UINT16(InvalidStrategy);
+ }
+}
the comments seem not right?

Re: SQL:2011 application time

[Paul Jungwirth]

On 5/13/24 03:11, Peter Eisentraut wrote:
It looks like we missed some of these fundamental design questions early on, and it might be too 
late now to fix them for PG17.


For example, the discussion on unique constraints misses that the question of null values in unique 
constraints itself is controversial and that there is now a way to change the behavior.  So I 
imagine there is also a selection of possible behaviors you might want for empty ranges.  
Intuitively, I don't think empty ranges are sensible for temporal unique constraints.  But anyway, 
it's a bit late now to be discussing this.


I'm also concerned that if ranges have this fundamental incompatibility with periods, then the plan 
to eventually evolve this patch set to support standard periods will also have as-yet-unknown problems.


Some of these issues might be design flaws in the underlying mechanisms, like range types and 
exclusion constraints.  Like, if you're supposed to use this for scheduling but you can use empty 
ranges to bypass exclusion constraints, how is one supposed to use this?  Yes, a check constraint 
using isempty() might be the right answer.  But I don't see this documented anywhere.


On the technical side, adding an implicit check constraint as part of a primary key constraint is 
quite a difficult implementation task, as I think you are discovering.  I'm just reminded about how 
the patch for catalogued not-null constraints struggled with linking these not-null constraints to 
primary keys correctly.  This sounds a bit similar.


I'm afraid that these issues cannot be resolved in good time for this release, so we should revert 
this patch set for now.


I think reverting is a good idea. I'm not really happy with the CHECK constraint solution either. 
I'd be happy to have some more time to rework this for v18.


A couple alternatives I'd like to explore:

1. Domain constraints instead of a CHECK constraint. I think this is probably worse, and I don't 
plan to spend much time on it, but I thought I'd mention it in case someone else thought otherwise.


2. A slightly different overlaps operator, say &&&, where 'empty' &&& 'empty' is true. But 'empty' 
with anything else could still be false (or not). That operator would prevent duplicates in an 
exclusion constraint. This also means we could support more types than just ranges & multiranges. I 
need to think about whether this combines badly with existing operators, but if not it has a lot of 
promise. If anything it might be *less* contradictory, because it fits better with 'empty' @> 
'empty', which we say is true.


Another thing a revert would give me some time to consider: even though it's not standard syntax, I 
wonder if we want to require syntax something like `PRIMARY KEY USING gist (id, valid_at WITHOUT 
OVERLAPS)`. Everywhere else we default to btree, so defaulting to gist feels a little weird. In 
theory we could even someday support WITHOUT OVERLAPS with btree, if we taught that AM to answer 
that question. (I admit there is probably not a lot of desire for that though.)


Yours,

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

Re: SQL:2011 application time

[Peter Eisentraut]

On 03.04.24 07:30, Paul Jungwirth wrote:
But is it *literally* unique? Well two identical keys, e.g. (5, 
'[Jan24,Mar24)') and (5, '[Jan24,Mar24)'), do have overlapping ranges, 
so the second is excluded. Normally a temporal unique index is *more* 
restrictive than a standard one, since it forbids other values too (e.g. 
(5, '[Jan24,Feb24)')). But sadly there is one exception: the ranges in 
these keys do not overlap: (5, 'empty'), (5, 'empty'). With 
ranges/multiranges, `'empty' && x` is false for all x. You can add that 
key as many times as you like, despite a PK/UQ constraint:


     postgres=# insert into t values
     ('[1,2)', 'empty', 'foo'),
     ('[1,2)', 'empty', 'bar');
     INSERT 0 2
     postgres=# select * from t;
   id   | valid_at | name
     ---+--+--
  [1,2) | empty    | foo
  [1,2) | empty    | bar
     (2 rows)

Cases like this shouldn't actually happen for temporal tables, since 
empty is not a meaningful value. An UPDATE/DELETE FOR PORTION OF would 
never cause an empty. But we should still make sure they don't cause 
problems.


We should give temporal primary keys an internal CHECK constraint saying 
`NOT isempty(valid_at)`. The problem is analogous to NULLs in parts of a 
primary key. NULLs prevent two identical keys from ever comparing as 
equal. And just as a regular primary key cannot contain NULLs, so a 
temporal primary key should not contain empties.


The standard effectively prevents this with PERIODs, because a PERIOD 
adds a constraint saying start < end. But our ranges enforce only start 
<= end. If you say `int4range(4,4)` you get `empty`. If we constrain 
primary keys as I'm suggesting, then they are literally unique, and 
indisunique seems safer.


Should we add the same CHECK constraint to temporal UNIQUE indexes? I'm 
inclined toward no, just as we don't forbid NULLs in parts of a UNIQUE 
key. We should try to pick what gives users more options, when possible. 
Even if it is questionably meaningful, I can see use cases for allowing 
empty ranges in a temporal table. For example it lets you "disable" a 
row, preserving its values but marking it as never true.


It looks like we missed some of these fundamental design questions early 
on, and it might be too late now to fix them for PG17.


For example, the discussion on unique constraints misses that the 
question of null values in unique constraints itself is controversial 
and that there is now a way to change the behavior.  So I imagine there 
is also a selection of possible behaviors you might want for empty 
ranges.  Intuitively, I don't think empty ranges are sensible for 
temporal unique constraints.  But anyway, it's a bit late now to be 
discussing this.


I'm also concerned that if ranges have this fundamental incompatibility 
with periods, then the plan to eventually evolve this patch set to 
support standard periods will also have as-yet-unknown problems.


Some of these issues might be design flaws in the underlying mechanisms, 
like range types and exclusion constraints.  Like, if you're supposed to 
use this for scheduling but you can use empty ranges to bypass exclusion 
constraints, how is one supposed to use this?  Yes, a check constraint 
using isempty() might be the right answer.  But I don't see this 
documented anywhere.


On the technical side, adding an implicit check constraint as part of a 
primary key constraint is quite a difficult implementation task, as I 
think you are discovering.  I'm just reminded about how the patch for 
catalogued not-null constraints struggled with linking these not-null 
constraints to primary keys correctly.  This sounds a bit similar.


I'm afraid that these issues cannot be resolved in good time for this 
release, so we should revert this patch set for now.

Re: SQL:2011 application time

[Paul Jungwirth]

On 5/12/24 08:51, Paul Jungwirth wrote:

On 5/12/24 05:55, Matthias van de Meent wrote:

  > pg=# CREATE UNIQUE INDEX ON temporal_testing USING gist (id, valid_during);
  > ERROR:  access method "gist" does not support unique indexes

To me that error message seems correct. The programmer hasn't said anything 
about the special
temporal behavior they are looking for.


But I showed that I had a GIST index that does have the indisunique
flag set, which shows that GIST does support indexes with unique
semantics.

That I can't use CREATE UNIQUE INDEX to create such an index doesn't
mean the feature doesn't exist, which is what the error message
implies.


True, the error message is not really telling the truth anymore. I do think most people who hit this 
error are not thinking about temporal constraints at all though, and for non-temporal constraints it 
is still true. It's also true for CREATE INDEX, since WITHOUT OVERLAPS is only available on the 
*constraint*. So how about adding a hint, something like this?:


ERROR:  access method "gist" does not support unique indexes
HINT: To create a unique constraint with non-overlap behavior, use ADD 
CONSTRAINT ... WITHOUT OVERLAPS.


I thought a little more about eventually implementing WITHOUT OVERLAPS support for CREATE INDEX, and 
how it relates to this error message in particular. Even when that is done, it will still depend on 
the stratnum support function for the keys' opclasses, so the GiST AM itself will still have false 
amcanunique, I believe. Probably the existing error message is still the right one. The hint won't 
need to mention ADD CONSTRAINT anymore. It should still point users to WITHOUT OVERLAPS, and 
possibly the stratnum support function too. I think what we are doing for v17 is all compatible with 
that plan.


Yours,

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

Re: SQL:2011 application time

[Paul Jungwirth]

On 5/5/24 20:01, jian he wrote:

hi.
I hope I understand the problem correctly.
my understanding is that we are trying to solve a corner case:
create table t(a int4range, b int4range, primary key(a, b WITHOUT OVERLAPS));
insert into t values ('[1,2]','empty'), ('[1,2]','empty');


I think the entry point is ATAddCheckNNConstraint and index_create.
in a chain of DDL commands, you cannot be sure which one
(primary key constraint or check constraint) is being created first,
you just want to make sure that after both constraints are created,
then add a dependency between primary key and check constraint.

so you need to validate at different functions
(ATAddCheckNNConstraint, index_create)
that these two constraints are indeed created,
only after that we have a dependency linking these two constraints.


I've attached a patch trying to solve this problem.
the patch is not totally polished, but works as expected, and also has
lots of comments.


Thanks for this! I've incorporated it into the CHECK constraint patch with some changes. In 
particular I thought index_create was a strange place to change the conperiod value of a 
pg_constraint record, and it is not actually needed if we are copying that value correctly.


Some other comments on the patch file:

> N.B. we also need to have special care for case
> where check constraint was readded, e.g. ALTER TYPE.
> if ALTER TYPE is altering the PERIOD column of the primary key,
> alter column of primary key makes the index recreate, check constraint 
recreate,
> however, former interally also including add a check constraint.
> so we need to take care of merging two check constraint.

This is a good point. I've included tests for this based on your patch.

> N.B. the check constraint name is hard-wired, so if you create the constraint
> with the same name, PERIOD primary key cannot be created.

Yes, it may be worth doing something like other auto-named constraints and trying to avoid 
duplicates. I haven't taken that on yet; I'm curious what others have to say about it.


> N.B. what about UNIQUE constraint?

See my previous posts on this thread about allowing 'empty' in UNIQUE 
constraints.

> N.B. seems ok to not care about FOREIGN KEY regarding this corner case?

Agreed.

v3 patches attached, rebased to 3ca43dbbb6.

Yours,

--
Paul  ~{:-)
p...@illuminatedcomputing.comFrom 4f4428fb41ea79056a13e425826fdac9c7b5d349 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" 
Date: Tue, 2 Apr 2024 15:39:04 -0700
Subject: [PATCH v3 1/2] Don't treat WITHOUT OVERLAPS indexes as unique in
 planner

Because the special rangetype 'empty' never overlaps another value, it
is possible for WITHOUT OVERLAPS tables to have two rows with the same
key, despite being indisunique, if the application-time range is
'empty'. So to be safe we should not treat WITHOUT OVERLAPS indexes as
unique in any proofs.

This still needs a test, but I'm having trouble finding a query that
gives wrong results.
---
 src/backend/optimizer/path/indxpath.c | 5 +++--
 src/backend/optimizer/plan/analyzejoins.c | 6 +++---
 src/backend/optimizer/util/plancat.c  | 1 +
 src/include/nodes/pathnodes.h | 2 ++
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index c0fcc7d78df..72346f78ebe 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -3498,13 +3498,14 @@ relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel,
 
 		/*
 		 * If the index is not unique, or not immediately enforced, or if it's
-		 * a partial index, it's useless here.  We're unable to make use of
+		 * a partial index, or if it's a WITHOUT OVERLAPS index (so not
+		 * literally unique), it's useless here.  We're unable to make use of
 		 * predOK partial unique indexes due to the fact that
 		 * check_index_predicates() also makes use of join predicates to
 		 * determine if the partial index is usable. Here we need proofs that
 		 * hold true before any joins are evaluated.
 		 */
-		if (!ind->unique || !ind->immediate || ind->indpred != NIL)
+		if (!ind->unique || !ind->immediate || ind->indpred != NIL || ind->hasperiod)
 			continue;
 
 		/*
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index c3fd4a81f8a..dc8327d5769 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -814,8 +814,8 @@ rel_supports_distinctness(PlannerInfo *root, RelOptInfo *rel)
 		 * For a plain relation, we only know how to prove uniqueness by
 		 * reference to unique indexes.  Make sure there's at least one
 		 * suitable unique index.  It must be immediately enforced, and not a
-		 * partial index. (Keep these conditions in sync with
-		 * relation_has_unique_index_for!)
+		 * partial index, and not WITHOUT OVERLAPS (Keep these conditions
+		 * in sync with relation_has_unique_index_for!)
 		 

Re: SQL:2011 application time

[Paul Jungwirth]

On 5/12/24 05:55, Matthias van de Meent wrote:

  > pg=# CREATE UNIQUE INDEX ON temporal_testing USING gist (id, valid_during);
  > ERROR:  access method "gist" does not support unique indexes

To me that error message seems correct. The programmer hasn't said anything 
about the special
temporal behavior they are looking for.


But I showed that I had a GIST index that does have the indisunique
flag set, which shows that GIST does support indexes with unique
semantics.

That I can't use CREATE UNIQUE INDEX to create such an index doesn't
mean the feature doesn't exist, which is what the error message
implies.


True, the error message is not really telling the truth anymore. I do think most people who hit this 
error are not thinking about temporal constraints at all though, and for non-temporal constraints it 
is still true. It's also true for CREATE INDEX, since WITHOUT OVERLAPS is only available on the 
*constraint*. So how about adding a hint, something like this?:


ERROR:  access method "gist" does not support unique indexes
HINT: To create a unique constraint with non-overlap behavior, use ADD 
CONSTRAINT ... WITHOUT OVERLAPS.

Yours,

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

Re: SQL:2011 application time

[Matthias van de Meent]

On Sun, 12 May 2024 at 05:26, Paul Jungwirth
 wrote:
> On 5/9/24 17:44, Matthias van de Meent wrote:
> > I haven't really been following this thread, but after playing around
> > a bit with the feature I feel there are new gaps in error messages. I
> > also think there are gaps in the functionality regarding the (lack of)
> > support for CREATE UNIQUE INDEX, and attaching these indexes to
> > constraints
> Thank you for trying this out and sharing your thoughts! I think these are 
> good points about CREATE
> UNIQUE INDEX and then creating the constraint by handing it an existing 
> index. This is something
> that I am hoping to add, but it's not covered by the SQL:2011 standard, so I 
> think it needs some
> discussion, and I don't think it needs to go into v17.

Okay.

> For instance you are saying:
>
>  > pg=# CREATE UNIQUE INDEX ON temporal_testing USING gist (id, valid_during);
>  > ERROR:  access method "gist" does not support unique indexes
>
> To me that error message seems correct. The programmer hasn't said anything 
> about the special
> temporal behavior they are looking for.

But I showed that I had a GIST index that does have the indisunique
flag set, which shows that GIST does support indexes with unique
semantics.

That I can't use CREATE UNIQUE INDEX to create such an index doesn't
mean the feature doesn't exist, which is what the error message
implies.

> To get non-overlapping semantics from an index, this more
> explicit syntax seems better, similar to PKs in the standard:

Yes, agreed on that part.

>  > pg=# CREATE UNIQUE INDEX ON temporal_testing USING gist (id, valid_during 
> WITHOUT OVERLAPS);
>  > ERROR:  access method "gist" does not support unique indexes
>
> We could also support *non-temporal* unique GiST indexes, particularly now 
> that we have the stratnum
> support function. Those would use the syntax you gave, omitting WITHOUT 
> OVERLAPS. But that seems
> like a separate effort to me.

No objection on that.

Kind regards,

Matthias van de Meent

Re: SQL:2011 application time

[Paul Jungwirth]

On 5/9/24 17:44, Matthias van de Meent wrote:

I haven't really been following this thread, but after playing around
a bit with the feature I feel there are new gaps in error messages. I
also think there are gaps in the functionality regarding the (lack of)
support for CREATE UNIQUE INDEX, and attaching these indexes to
constraints
Thank you for trying this out and sharing your thoughts! I think these are good points about CREATE 
UNIQUE INDEX and then creating the constraint by handing it an existing index. This is something 
that I am hoping to add, but it's not covered by the SQL:2011 standard, so I think it needs some 
discussion, and I don't think it needs to go into v17.


For instance you are saying:

> pg=# CREATE UNIQUE INDEX ON temporal_testing USING gist (id, valid_during);
> ERROR:  access method "gist" does not support unique indexes

To me that error message seems correct. The programmer hasn't said anything about the special 
temporal behavior they are looking for. To get non-overlapping semantics from an index, this more 
explicit syntax seems better, similar to PKs in the standard:


> pg=# CREATE UNIQUE INDEX ON temporal_testing USING gist (id, valid_during 
WITHOUT OVERLAPS);
> ERROR:  access method "gist" does not support unique indexes

We could also support *non-temporal* unique GiST indexes, particularly now that we have the stratnum 
support function. Those would use the syntax you gave, omitting WITHOUT OVERLAPS. But that seems 
like a separate effort to me.



Additionally, because I can't create my own non-constraint-backing
unique GIST indexes, I can't pre-create my unique constraints
CONCURRENTLY as one could do for the non-temporal case: UNIQUE
constraints hold ownership of the index and would drop the index if
the constraint is dropped, too, and don't support a CONCURRENTLY
modifier, nor an INVALID modifier. This means temporal unique
constraints have much less administrative wiggle room than normal
unique constraints, and I think that's not great.


This is a great use-case for why we should support this eventually, even if it 
uses non-standard syntax.

Yours,

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

Re: SQL:2011 application time

[Paul Jungwirth]

On 5/11/24 17:00, jian he wrote:

I hope I understand the problem correctly.
my understanding is that we are trying to solve a corner case:
create table t(a int4range, b int4range, primary key(a, b WITHOUT OVERLAPS));
insert into t values ('[1,2]','empty'), ('[1,2]','empty');




but we still not yet address for cases like:
create table t10(a int4range, b int4range, unique (a, b WITHOUT OVERLAPS));
insert into t10 values ('[1,2]','empty'), ('[1,2]','empty');

one table can have more than one temporal unique constraint,
for each temporal unique constraint adding a check isempty constraint
seems not easy.


I think we should add the not-empty constraint only for PRIMARY KEYs, not all UNIQUE constraints. 
The empty edge case is very similar to the NULL edge case, and while every PK column must be 
non-null, we do allow nulls in ordinary UNIQUE constraints. If users want to have 'empty' in those 
constraints, I think we should let them. And then the problems you give don't arise.



Maybe we can just mention that the special 'empty' range value makes
temporal unique constraints not "unique".


Just documenting the behavior is also an okay solution here I think. I see two downsides though: (1) 
it makes rangetype temporal keys differ from PERIOD temporal keys (2) it could allow more 
planner/etc bugs than we have thought of. So I think it's worth adding the constraint instead.



also we can make sure that
FOREIGN KEY can only reference primary keys, not unique temporal constraints.
so the unique temporal constraints not "unique" implication is limited.
I played around with it, we can error out these cases in the function
transformFkeyCheckAttrs.


I don't think it is a problem to reference a temporal UNIQUE constraint, even if it contains empty 
values. An empty value means you're not asserting that row at any time (though another row might 
assert the same thing for some time), so it could never contribute toward fulfilling a reference anyway.


I do think it would be nice if the *reference* could contain empty values. Right now the FK SQL will 
cause that to never match, because we use `&&` as an optimization, but we could tweak the SQL (maybe 
for v18 instead) so that users could get away with that kind of thing. As I said in an earlier 
email, this would be you an escape hatch to reference a temporal table from a non-temporal table. 
Otherwise temporal tables are "contagious," which is a bit of a drawback.


Yours,

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

Re: SQL:2011 application time

[jian he]

On Mon, May 6, 2024 at 11:01 AM jian he  wrote:
>
> On Wed, May 1, 2024 at 12:39 AM Paul Jungwirth
>  wrote:
> >
> > On 4/30/24 09:24, Robert Haas wrote:
> > > Peter, could you have a look at
> > > http://postgr.es/m/47550967-260b-4180-9791-b224859fe...@illuminatedcomputing.com
> > > and express an opinion about whether each of those proposals are (a)
> > > good or bad ideas and (b) whether they need to be fixed for the
> > > current release?
> >
> > Here are the same patches but rebased. I've added a fourth which is my 
> > progress on adding the CHECK
> > constraint. I don't really consider it finished though, because it has 
> > these problems:
> >
> > - The CHECK constraint should be marked as an internal dependency of the 
> > PK, so that you can't drop
> > it, and it gets dropped when you drop the PK. I don't see a good way to tie 
> > the two together though,
> > so I'd appreciate any advice there. They are separate AlterTableCmds, so 
> > how do I get the
> > ObjectAddress of both constraints at the same time? I wanted to store the 
> > PK's ObjectAddress on the
> > Constraint node, but since ObjectAddress isn't a Node it doesn't work.
> >
>
> hi.
> I hope I understand the problem correctly.
> my understanding is that we are trying to solve a corner case:
> create table t(a int4range, b int4range, primary key(a, b WITHOUT OVERLAPS));
> insert into t values ('[1,2]','empty'), ('[1,2]','empty');
>


but we still not yet address for cases like:
create table t10(a int4range, b int4range, unique (a, b WITHOUT OVERLAPS));
insert into t10 values ('[1,2]','empty'), ('[1,2]','empty');

one table can have more than one temporal unique constraint,
for each temporal unique constraint adding a check isempty constraint
seems not easy.

for example:
CREATE TABLE t (
id int4range,
valid_at daterange,
parent_id int4range,
CONSTRAINT t1 unique (id, valid_at WITHOUT OVERLAPS),
CONSTRAINT t2 unique (parent_id, valid_at WITHOUT OVERLAPS),
CONSTRAINT t3 unique (valid_at, id WITHOUT OVERLAPS),
CONSTRAINT t4 unique (parent_id, id WITHOUT OVERLAPS),
CONSTRAINT t5 unique (id, parent_id WITHOUT OVERLAPS),
CONSTRAINT t6 unique (valid_at, parent_id WITHOUT OVERLAPS)
);
add 6 check isempty constraints for table "t"  is challenging.

so far, I see the challenging part:
* alter table alter column data type does not drop previous check
isempty constraint, and will also add a check isempty constraint,
so overall it will add more check constraints.
* adding more check constraints needs a  way to resolve naming collisions.

Maybe we can just mention that the special 'empty' range value makes
temporal unique constraints not "unique".

also we can make sure that
FOREIGN KEY can only reference primary keys, not unique temporal constraints.
so the unique temporal constraints not "unique" implication is limited.
I played around with it, we can error out these cases in the function
transformFkeyCheckAttrs.

Re: SQL:2011 application time

[Peter Eisentraut]

I have committed the 
v2-0001-Fix-ON-CONFLICT-DO-NOTHING-UPDATE-for-temporal-in.patch from 
this (confusingly, there was also a v2 earlier in this thread), and I'll 
continue working on the remaining items.



On 09.05.24 06:24, Paul Jungwirth wrote:
Here are a couple new patches, rebased to e305f715, addressing Peter's 
feedback. I'm still working on integrating jian he's suggestions for the 
last patch, so I've omitted that one here.


On 5/8/24 06:51, Peter Eisentraut wrote:
About v2-0001-Fix-ON-CONFLICT-DO-NOTHING-UPDATE-for-temporal-in.patch, 
I think the
ideas are right, but I wonder if we can fine-tune the new conditionals 
a bit.


--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -210,7 +210,7 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo, bool 
speculative)
  * If the indexes are to be used for speculative 
insertion, add extra

  * information required by unique index entries.
  */
-   if (speculative && ii->ii_Unique)
+   if (speculative && ii->ii_Unique && 
!ii->ii_HasWithoutOverlaps)

 BuildSpeculativeIndexInfo(indexDesc, ii);

Here, I think we could check !indexDesc->rd_index->indisexclusion 
instead.  So we

wouldn't need ii_HasWithoutOverlaps.


Okay.

Or we could push this into BuildSpeculativeIndexInfo(); it could just 
skip the rest
if an exclusion constraint is passed, on the theory that all the 
speculative index

info is already present in that case.


I like how BuildSpeculativeIndexInfo starts with an Assert that it's 
given a unique index, so I've left the check outside the function. This 
seems cleaner anyway: the function stays more focused.



--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -815,7 +815,7 @@ infer_arbiter_indexes(PlannerInfo *root)
  */
 if (indexOidFromConstraint == idxForm->indexrelid)
 {
-   if (!idxForm->indisunique && onconflict->action == 
ONCONFLICT_UPDATE)
+   if ((!idxForm->indisunique || idxForm->indisexclusion) && 
onconflict->action == ONCONFLICT_UPDATE)

 ereport(ERROR,
 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
  errmsg("ON CONFLICT DO UPDATE not supported 
with exclusion constraints")));


Shouldn't this use only idxForm->indisexclusion anyway?  Like

+   if (idxForm->indisexclusion && onconflict->action == 
ONCONFLICT_UPDATE)


That matches what the error message is reporting afterwards.


Agreed.


  * constraints), so index under consideration can be immediately
  * skipped if it's not unique
  */
-   if (!idxForm->indisunique)
+   if (!idxForm->indisunique || idxForm->indisexclusion)
 goto next;

Maybe here we need a comment.  Or make that a separate statement, like


Yes, that is nice. Done.

Yours,

Re: SQL:2011 application time

[Matthias van de Meent]

Hi,

I haven't really been following this thread, but after playing around
a bit with the feature I feel there are new gaps in error messages. I
also think there are gaps in the functionality regarding the (lack of)
support for CREATE UNIQUE INDEX, and attaching these indexes to
constraints.

pg=# CREATE TABLE temporal_testing (
pg(#  id bigint NOT NULL
pg(#generated always as identity,
pg(#  valid_during tstzrange
pg(# );
CREATE TABLE
pg=# ALTER TABLE temporal_testing
pg-#  ADD CONSTRAINT temp_unique UNIQUE (id, valid_during WITHOUT OVERLAPS);
ALTER TABLE
pg=# \d+ temp_unique
 Index "public.temp_unique"
Column|Type | Key? |  Definition  | Storage  | Stats target
--+-+--+--+--+--
 id   | gbtreekey16 | yes  | id   | plain|
 valid_during | tstzrange   | yes  | valid_during | extended |
unique, gist, for table "public.temporal_testing"
-- ^^ note the "unique, gist"
pg=# CREATE UNIQUE INDEX ON temporal_testing USING gist (id, valid_during);
ERROR:  access method "gist" does not support unique indexes

Here we obviously have a unique GIST index in the catalogs, but
they're "not supported" by GIST when we try to create such index
ourselves (!). Either the error message needs updating, or we need to
have a facility to actually support creating these unique indexes
outside constraints.

Additionally, because I can't create my own non-constraint-backing
unique GIST indexes, I can't pre-create my unique constraints
CONCURRENTLY as one could do for the non-temporal case: UNIQUE
constraints hold ownership of the index and would drop the index if
the constraint is dropped, too, and don't support a CONCURRENTLY
modifier, nor an INVALID modifier. This means temporal unique
constraints have much less administrative wiggle room than normal
unique constraints, and I think that's not great.

Kind regards,

Matthias van de Meent.

Re: SQL:2011 application time

[Paul Jungwirth]

Here are a couple new patches, rebased to e305f715, addressing Peter's feedback. I'm still working 
on integrating jian he's suggestions for the last patch, so I've omitted that one here.


On 5/8/24 06:51, Peter Eisentraut wrote:

About v2-0001-Fix-ON-CONFLICT-DO-NOTHING-UPDATE-for-temporal-in.patch, I think 
the
ideas are right, but I wonder if we can fine-tune the new conditionals a bit.

--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -210,7 +210,7 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo, bool 
speculative)
  * If the indexes are to be used for speculative insertion, 
add extra
  * information required by unique index entries.
  */
-   if (speculative && ii->ii_Unique)
+   if (speculative && ii->ii_Unique && !ii->ii_HasWithoutOverlaps)
     BuildSpeculativeIndexInfo(indexDesc, ii);

Here, I think we could check !indexDesc->rd_index->indisexclusion instead.  So 
we
wouldn't need ii_HasWithoutOverlaps.


Okay.


Or we could push this into BuildSpeculativeIndexInfo(); it could just skip the 
rest
if an exclusion constraint is passed, on the theory that all the speculative 
index
info is already present in that case.


I like how BuildSpeculativeIndexInfo starts with an Assert that it's given a unique index, so I've 
left the check outside the function. This seems cleaner anyway: the function stays more focused.



--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -815,7 +815,7 @@ infer_arbiter_indexes(PlannerInfo *root)
  */
     if (indexOidFromConstraint == idxForm->indexrelid)
     {
-   if (!idxForm->indisunique && onconflict->action == 
ONCONFLICT_UPDATE)
+   if ((!idxForm->indisunique || idxForm->indisexclusion) && onconflict->action == 
ONCONFLICT_UPDATE)

     ereport(ERROR,
     (errcode(ERRCODE_WRONG_OBJECT_TYPE),
  errmsg("ON CONFLICT DO UPDATE not supported with exclusion 
constraints")));

Shouldn't this use only idxForm->indisexclusion anyway?  Like

+   if (idxForm->indisexclusion && onconflict->action == 
ONCONFLICT_UPDATE)

That matches what the error message is reporting afterwards.


Agreed.


  * constraints), so index under consideration can be immediately
  * skipped if it's not unique
  */
-   if (!idxForm->indisunique)
+   if (!idxForm->indisunique || idxForm->indisexclusion)
     goto next;

Maybe here we need a comment.  Or make that a separate statement, like


Yes, that is nice. Done.

Yours,

--
Paul  ~{:-)
p...@illuminatedcomputing.comFrom 0ebe2e6d6cd8dc6f8120fe93b9024cf80472f8cc Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" 
Date: Sun, 24 Mar 2024 21:46:30 -0700
Subject: [PATCH v2 1/2] Fix ON CONFLICT DO NOTHING/UPDATE for temporal indexes

A PRIMARY KEY or UNIQUE constraint with WITHOUT OVERLAPS will be a GiST
index, not a B-Tree, but it will still have indisunique set. The code
for ON CONFLICT fails if it sees a non-btree index that has indisunique.
This commit fixes that and adds some tests. But now that we can't just
test indisunique, we also need some extra checks to prevent DO UPDATE
from running against a WITHOUT OVERLAPS constraint (because the conflict
could happen against more than one row, and we'd only update one).
---
 src/backend/catalog/index.c   |   1 +
 src/backend/executor/execIndexing.c   |   2 +-
 src/backend/optimizer/util/plancat.c  |   9 +-
 src/include/nodes/execnodes.h |   1 +
 .../regress/expected/without_overlaps.out | 176 ++
 src/test/regress/sql/without_overlaps.sql | 113 +++
 6 files changed, 300 insertions(+), 2 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5a8568c55c9..1fd543cc550 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2443,6 +2443,7 @@ BuildIndexInfo(Relation index)
  >ii_ExclusionOps,
  >ii_ExclusionProcs,
  >ii_ExclusionStrats);
+		ii->ii_HasWithoutOverlaps = ii->ii_Unique;
 	}
 
 	return ii;
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 9f05b3654c1..59acf67a36a 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -210,7 +210,7 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo, bool speculative)
 		 * If the indexes are to be used for speculative insertion, add extra
 		 * information required by unique index entries.
 		 */
-		if (speculative && ii->ii_Unique)
+		if (speculative && ii->ii_Unique && !indexDesc->rd_index->indisexclusion)
 			BuildSpeculativeIndexInfo(indexDesc, ii);
 
 		relationDescs[i] = indexDesc;
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 130f838629f..775c3e26cd8 100644
--- 

Re: SQL:2011 application time

[Peter Eisentraut]

On 30.04.24 18:39, Paul Jungwirth wrote:

On 4/30/24 09:24, Robert Haas wrote:

Peter, could you have a look at
http://postgr.es/m/47550967-260b-4180-9791-b224859fe...@illuminatedcomputing.com
and express an opinion about whether each of those proposals are (a)
good or bad ideas and (b) whether they need to be fixed for the
current release?


Here are the same patches but rebased.


I have committed 
v2-0002-Add-test-for-REPLICA-IDENTITY-with-a-temporal-key.patch.

About v2-0001-Fix-ON-CONFLICT-DO-NOTHING-UPDATE-for-temporal-in.patch, I think 
the
ideas are right, but I wonder if we can fine-tune the new conditionals a bit.

--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -210,7 +210,7 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo, bool 
speculative)
 * If the indexes are to be used for speculative insertion, add 
extra
 * information required by unique index entries.
 */
-   if (speculative && ii->ii_Unique)
+   if (speculative && ii->ii_Unique && !ii->ii_HasWithoutOverlaps)
BuildSpeculativeIndexInfo(indexDesc, ii);

Here, I think we could check !indexDesc->rd_index->indisexclusion instead.  So 
we
wouldn't need ii_HasWithoutOverlaps.

Or we could push this into BuildSpeculativeIndexInfo(); it could just skip the 
rest
if an exclusion constraint is passed, on the theory that all the speculative 
index
info is already present in that case.

--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -815,7 +815,7 @@ infer_arbiter_indexes(PlannerInfo *root)
 */
if (indexOidFromConstraint == idxForm->indexrelid)
{
-   if (!idxForm->indisunique && onconflict->action == 
ONCONFLICT_UPDATE)
+   if ((!idxForm->indisunique || idxForm->indisexclusion) && 
onconflict->action == ONCONFLICT_UPDATE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 errmsg("ON CONFLICT DO UPDATE not supported with exclusion 
constraints")));

Shouldn't this use only idxForm->indisexclusion anyway?  Like

+   if (idxForm->indisexclusion && onconflict->action == 
ONCONFLICT_UPDATE)

That matches what the error message is reporting afterwards.

 * constraints), so index under consideration can be immediately
 * skipped if it's not unique
 */
-   if (!idxForm->indisunique)
+   if (!idxForm->indisunique || idxForm->indisexclusion)
goto next;

Maybe here we need a comment.  Or make that a separate statement, like

/* not supported yet etc. */
if (idxForm->indixexclusion)
next;

Re: SQL:2011 application time

[jian he]

On Wed, May 1, 2024 at 12:39 AM Paul Jungwirth
 wrote:
>
> On 4/30/24 09:24, Robert Haas wrote:
> > Peter, could you have a look at
> > http://postgr.es/m/47550967-260b-4180-9791-b224859fe...@illuminatedcomputing.com
> > and express an opinion about whether each of those proposals are (a)
> > good or bad ideas and (b) whether they need to be fixed for the
> > current release?
>
> Here are the same patches but rebased. I've added a fourth which is my 
> progress on adding the CHECK
> constraint. I don't really consider it finished though, because it has these 
> problems:
>
> - The CHECK constraint should be marked as an internal dependency of the PK, 
> so that you can't drop
> it, and it gets dropped when you drop the PK. I don't see a good way to tie 
> the two together though,
> so I'd appreciate any advice there. They are separate AlterTableCmds, so how 
> do I get the
> ObjectAddress of both constraints at the same time? I wanted to store the 
> PK's ObjectAddress on the
> Constraint node, but since ObjectAddress isn't a Node it doesn't work.
>

hi.
I hope I understand the problem correctly.
my understanding is that we are trying to solve a corner case:
create table t(a int4range, b int4range, primary key(a, b WITHOUT OVERLAPS));
insert into t values ('[1,2]','empty'), ('[1,2]','empty');


I think the entry point is ATAddCheckNNConstraint and index_create.
in a chain of DDL commands, you cannot be sure which one
(primary key constraint or check constraint) is being created first,
you just want to make sure that after both constraints are created,
then add a dependency between primary key and check constraint.

so you need to validate at different functions
(ATAddCheckNNConstraint, index_create)
that these two constraints are indeed created,
only after that we have a dependency linking these two constraints.


I've attached a patch trying to solve this problem.
the patch is not totally polished, but works as expected, and also has
lots of comments.
From 2028de0384b81bb9b2bff53fd391b08f57aba242 Mon Sep 17 00:00:00 2001
From: jian he 
Date: Mon, 6 May 2024 10:12:26 +0800
Subject: [PATCH v4 1/1] add a special check constrint for PERIOD primary key

last column of PERIOD primary key cannot have empty value,
otherwise primary key may lost uniqueness property.

corner case demo:
create table t(a int4range, b int4range, primary key(a, b WITHOUT OVERLAPS));
insert into t values ('[1,2]','empty'), ('[1,2]','empty');

this patch makes it fails by internally add a check constraint:
CHECK (NOT isempty(period_column))
after that, the table `t` will look like:

 Column |   Type| Collation | Nullable | Default
+---+---+--+-
 a  | int4range |   | not null |
 b  | int4range |   | not null |
Indexes:
"t_pkey" PRIMARY KEY (a, b WITHOUT OVERLAPS)
Check constraints:
"b_not_empty" CHECK (NOT isempty(b))

to distinguish this constraint with other check constraint, we
make it conperiod as true in pg_constraint catalog.

we aslo add a internal dependency between the primary key constraint
and this check constraint.
so you cannot drop the check constraint itself,
if you drop the primary key constraint, this check constraint
will be dropped automatically.

generally we add check constraint within the function ATAddCheckNNConstraint,
primary key constraint within the function index_create.
in a chain of DDL command, we are not sure which one is be
first created, to make it safe, we do cross check at these two
functions, after both check constraint and primary key constraint
are created, then we add a internal dependencies on it.

N.B. we also need to have special care for case
where check constraint was readded, e.g. ALTER TYPE.
if ALTER TYPE is altering the PERIOD column of the primary key,
alter column of primary key makes the index recreate, check constraint recreate,
however, former interally also including add a check constraint.
so we need to take care of merging two check constraint.

N.B. the check constraint name is hard-wired, so if you create the constraint
with the same name, PERIOD primary key cannot be created.

N.B. what about UNIQUE constraint?

N.B. seems ok to not care about FOREIGN KEY regarding this corner case?

Discussion: https://postgr.es/m/3775839b-3f0f-4c8a-ac03-a253222e6...@illuminatedcomputing.com
---
 doc/src/sgml/gist.sgml|   3 -
 doc/src/sgml/ref/create_table.sgml|   5 +-
 src/backend/catalog/heap.c|  10 +-
 src/backend/catalog/index.c   |  22 ++
 src/backend/commands/tablecmds.c  | 188 +
 src/backend/parser/parse_utilcmd.c|  88 +-
 src/include/commands/tablecmds.h  |   4 +
 .../regress/expected/without_overlaps.out | 251 +-
 src/test/regress/sql/without_overlaps.sql | 120 +
 9 files changed, 664 insertions(+), 27 deletions(-)

diff --git a/doc/src/sgml/gist.sgml 

Re: SQL:2011 application time

[jian he]

On Wed, May 1, 2024 at 12:39 AM Paul Jungwirth
 wrote:
>
> On 4/30/24 09:24, Robert Haas wrote:
> > Peter, could you have a look at
> > http://postgr.es/m/47550967-260b-4180-9791-b224859fe...@illuminatedcomputing.com
> > and express an opinion about whether each of those proposals are (a)
> > good or bad ideas and (b) whether they need to be fixed for the
> > current release?
>
> Here are the same patches but rebased. I've added a fourth which is my 
> progress on adding the CHECK
> constraint. I don't really consider it finished though, because it has these 
> problems:
>
> - The CHECK constraint should be marked as an internal dependency of the PK, 
> so that you can't drop
> it, and it gets dropped when you drop the PK. I don't see a good way to tie 
> the two together though,
> so I'd appreciate any advice there. They are separate AlterTableCmds, so how 
> do I get the
> ObjectAddress of both constraints at the same time? I wanted to store the 
> PK's ObjectAddress on the
> Constraint node, but since ObjectAddress isn't a Node it doesn't work.
>
> - The CHECK constraint should maybe be hidden when you say `\d foo`? Or maybe 
> not, but that's what
> we do with FK triggers.
>
> - When you create partitions you get a warning about the constraint already 
> existing, because it
> gets created via the PK and then also the partitioning code tries to copy it. 
> Solving the first
> issue here should solve this nicely though.
>
> Alternately we could just fix the GROUP BY functional dependency code to only 
> accept b-tree indexes.
> But I think the CHECK constraint approach is a better solution.
>

I will consider these issues later.
The following are general ideas after applying your patches.

CREATE TABLE temporal_rng1(
id int4range,
valid_at daterange,
CONSTRAINT temporal_rng1_pk unique (id, valid_at WITHOUT OVERLAPS)
);
insert into temporal_rng1(id, valid_at) values (int4range '[1,1]',
'empty'::daterange), ('[1,1]', 'empty');
table temporal_rng1;
  id   | valid_at
---+--
 [1,2) | empty
 [1,2) | empty
(2 rows)

i hope i didn't miss something:
exclude the 'empty' special value, WITHOUT OVERLAP constraint will be
unique and is more restrictive?

if so,
then adding a check constraint to make the WITHOUT OVERLAP not include
the special value 'empty'
is better than
writing a doc explaining that on some special occasion, a unique
constraint is not meant to be unique
?

in here
https://www.postgresql.org/docs/devel/ddl-constraints.html#DDL-CONSTRAINTS-UNIQUE-CONSTRAINTS
says:
<<
Unique constraints ensure that the data contained in a column, or a
group of columns, is unique among all the rows in the table.
<<

+ /*
+ * The WITHOUT OVERLAPS part (if any) must be
+ * a range or multirange type.
+ */
+ if (constraint->without_overlaps && lc == list_last_cell(constraint->keys))
+ {
+ Oid typid = InvalidOid;
+
+ if (!found && cxt->isalter)
+ {
+ /*
+ * Look up the column type on existing table.
+ * If we can't find it, let things fail in DefineIndex.
+ */
+ Relation rel = cxt->rel;
+ for (int i = 0; i < rel->rd_att->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(rel->rd_att, i);
+ const char *attname;
+
+ if (attr->attisdropped)
+ break;
+
+ attname = NameStr(attr->attname);
+ if (strcmp(attname, key) == 0)
+ {
+ typid = attr->atttypid;
+ break;
+ }
+ }
+ }
+ else
+ typid = typenameTypeId(NULL, column->typeName);
+
+ if (OidIsValid(typid) && !type_is_range(typid) && !type_is_multirange(typid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("column \"%s\" in WITHOUT OVERLAPS is not a range or
multirange type", key),
+ parser_errposition(cxt->pstate, constraint->location)));
+ }

+ if (attr->attisdropped)
+ break;
it will break the loop?
but here you want to continue the loop?

+ if (OidIsValid(typid) && !type_is_range(typid) && !type_is_multirange(typid))
didn't consider the case where typid is InvalidOid,
maybe we can simplify to
+ if (!type_is_range(typid) && !type_is_multirange(typid))


+ notnullcmds = lappend(notnullcmds, notemptycmd);
seems weird.
we can imitate notnullcmds related logic for notemptycmd,
not associated notnullcmds in any way.

Re: SQL:2011 application time

[Paul Jungwirth]

On 4/30/24 09:24, Robert Haas wrote:

Peter, could you have a look at
http://postgr.es/m/47550967-260b-4180-9791-b224859fe...@illuminatedcomputing.com
and express an opinion about whether each of those proposals are (a)
good or bad ideas and (b) whether they need to be fixed for the
current release?


Here are the same patches but rebased. I've added a fourth which is my progress on adding the CHECK 
constraint. I don't really consider it finished though, because it has these problems:


- The CHECK constraint should be marked as an internal dependency of the PK, so that you can't drop 
it, and it gets dropped when you drop the PK. I don't see a good way to tie the two together though, 
so I'd appreciate any advice there. They are separate AlterTableCmds, so how do I get the 
ObjectAddress of both constraints at the same time? I wanted to store the PK's ObjectAddress on the 
Constraint node, but since ObjectAddress isn't a Node it doesn't work.


- The CHECK constraint should maybe be hidden when you say `\d foo`? Or maybe not, but that's what 
we do with FK triggers.


- When you create partitions you get a warning about the constraint already existing, because it 
gets created via the PK and then also the partitioning code tries to copy it. Solving the first 
issue here should solve this nicely though.


Alternately we could just fix the GROUP BY functional dependency code to only accept b-tree indexes. 
But I think the CHECK constraint approach is a better solution.


Thanks,

--
Paul  ~{:-)
p...@illuminatedcomputing.comFrom 0dbc008a654ab1fdc5f492345ee4575c352499d3 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" 
Date: Sun, 24 Mar 2024 21:46:30 -0700
Subject: [PATCH v2 1/4] Fix ON CONFLICT DO NOTHING/UPDATE for temporal indexes

A PRIMARY KEY or UNIQUE constraint with WITHOUT OVERLAPS will be a GiST
index, not a B-Tree, but it will still have indisunique set. The code
for ON CONFLICT fails if it sees a non-btree index that has indisunique.
This commit fixes that and adds some tests. But now that we can't just
test indisunique, we also need some extra checks to prevent DO UPDATE
from running against a WITHOUT OVERLAPS constraint (because the conflict
could happen against more than one row, and we'd only update one).
---
 src/backend/catalog/index.c   |   1 +
 src/backend/executor/execIndexing.c   |   2 +-
 src/backend/optimizer/util/plancat.c  |   4 +-
 src/include/nodes/execnodes.h |   1 +
 .../regress/expected/without_overlaps.out | 176 ++
 src/test/regress/sql/without_overlaps.sql | 113 +++
 6 files changed, 294 insertions(+), 3 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5a8568c55c9..1fd543cc550 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2443,6 +2443,7 @@ BuildIndexInfo(Relation index)
  >ii_ExclusionOps,
  >ii_ExclusionProcs,
  >ii_ExclusionStrats);
+		ii->ii_HasWithoutOverlaps = ii->ii_Unique;
 	}
 
 	return ii;
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 9f05b3654c1..faa37ca56db 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -210,7 +210,7 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo, bool speculative)
 		 * If the indexes are to be used for speculative insertion, add extra
 		 * information required by unique index entries.
 		 */
-		if (speculative && ii->ii_Unique)
+		if (speculative && ii->ii_Unique && !ii->ii_HasWithoutOverlaps)
 			BuildSpeculativeIndexInfo(indexDesc, ii);
 
 		relationDescs[i] = indexDesc;
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 130f838629f..a398d7a78d1 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -815,7 +815,7 @@ infer_arbiter_indexes(PlannerInfo *root)
 		 */
 		if (indexOidFromConstraint == idxForm->indexrelid)
 		{
-			if (!idxForm->indisunique && onconflict->action == ONCONFLICT_UPDATE)
+			if ((!idxForm->indisunique || idxForm->indisexclusion) && onconflict->action == ONCONFLICT_UPDATE)
 ereport(ERROR,
 		(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 		 errmsg("ON CONFLICT DO UPDATE not supported with exclusion constraints")));
@@ -837,7 +837,7 @@ infer_arbiter_indexes(PlannerInfo *root)
 		 * constraints), so index under consideration can be immediately
 		 * skipped if it's not unique
 		 */
-		if (!idxForm->indisunique)
+		if (!idxForm->indisunique || idxForm->indisexclusion)
 			goto next;
 
 		/* Build BMS representation of plain (non expression) index attrs */
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d927ac44a82..fdfaef284e9 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -204,6 +204,7 @@ typedef struct IndexInfo
 	bool		ii_Summarizing;
 	int			ii_ParallelWorkers;
 	Oid			ii_Am;
+	bool	

Re: SQL:2011 application time

[Robert Haas]

On Fri, Apr 26, 2024 at 3:41 PM Paul Jungwirth
 wrote:
> On 4/26/24 12:25, Robert Haas wrote:
> > I think this thread should be added to the open items list.
>
> Thanks! I sent a request to pgsql-www to get edit permission. I didn't 
> realize there was a wiki page
> tracking things like this. I agree it needs to be fixed if we want to include 
> the feature.

Great, I see that it's on the list now.

Peter, could you have a look at
http://postgr.es/m/47550967-260b-4180-9791-b224859fe...@illuminatedcomputing.com
and express an opinion about whether each of those proposals are (a)
good or bad ideas and (b) whether they need to be fixed for the
current release?

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: SQL:2011 application time

[Paul Jungwirth]

On 4/26/24 12:25, Robert Haas wrote:

I think this thread should be added to the open items list.


Thanks! I sent a request to pgsql-www to get edit permission. I didn't realize there was a wiki page 
tracking things like this. I agree it needs to be fixed if we want to include the feature.


Yours,

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

Re: SQL:2011 application time

[Robert Haas]

On Wed, Apr 3, 2024 at 1:30 AM Paul Jungwirth
 wrote:
> I found some problems with temporal primary keys and the idea of uniqueness, 
> especially around the
> indisunique column. Here are some small fixes and a proposal for a larger 
> fix, which I think we need
> but I'd like some feedback on.

I think this thread should be added to the open items list. You're
raising questions about whether the feature that was committed to this
release is fully correct. If it isn't, we shouldn't release it without
fixing it.

https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: SQL:2011 application time

[jian he]

On Wed, Apr 3, 2024 at 1:30 PM Paul Jungwirth
 wrote:
>
> On 3/24/24 00:38, Peter Eisentraut wrote:> I have committed the patches
> > v33-0001-Add-temporal-FOREIGN-KEYs.patch and 
> > v33-0002-Support-multiranges-in-temporal-FKs.patch
> > (together).
>
> Hi Hackers,
>
> I found some problems with temporal primary keys and the idea of uniqueness, 
> especially around the
> indisunique column. Here are some small fixes and a proposal for a larger 
> fix, which I think we need
> but I'd like some feedback on.
>
> The first patch fixes problems with ON CONFLICT DO NOTHING/UPDATE.
>
> DO NOTHING fails because it doesn't expect a non-btree unique index. It's 
> fine to make it accept a
> temporal PRIMARY KEY/UNIQUE index though (i.e. an index with both indisunique 
> and indisexclusion).
> This is no different than an exclusion constraint. So I skip 
> BuildSpeculativeIndexInfo for WITHOUT
> OVERLAPS indexes. (Incidentally, AFAICT ii_UniqueOps is never used, only 
> ii_UniqueProcs. Right?)
>

hi.
for unique index, primary key:
ii_ExclusionOps, ii_UniqueOps is enough to distinguish this index
support without overlaps,
we don't need another ii_HasWithoutOverlaps?
(i didn't test it though)


ON CONFLICT DO NOTHING
ON CONFLICT (id, valid_at) DO NOTHING
ON CONFLICT ON CONSTRAINT temporal_rng_pk DO NOTHING
I am confused by the test.
here temporal_rng only has one primary key, ON CONFLICT only deals with it.
I thought these three are the same thing?


DROP TABLE temporal_rng;
CREATE TABLE temporal_rng (id int4range,valid_at daterange);
ALTER TABLE temporal_rng ADD CONSTRAINT temporal_rng_pk PRIMARY KEY
(id, valid_at WITHOUT OVERLAPS);

+-- ON CONFLICT
+--
+TRUNCATE temporal_rng;
+INSERT INTO temporal_rng (id, valid_at) VALUES ('[1,2)',
daterange('2000-01-01', '2010-01-01'));
+-- with a conflict
+INSERT INTO temporal_rng (id, valid_at) VALUES ('[1,2)',
daterange('2005-01-01', '2006-01-01')) ON CONFLICT DO NOTHING;
+-- id matches but no conflict

+TRUNCATE temporal_rng;
+INSERT INTO temporal_rng (id, valid_at) VALUES ('[1,2)',
daterange('2000-01-01', '2010-01-01'));
+-- with a conflict
+INSERT INTO temporal_rng (id, valid_at) VALUES ('[1,2)',
daterange('2005-01-01', '2006-01-01')) ON CONFLICT (id, valid_at) DO
NOTHING;
+ERROR:  there is no unique or exclusion constraint matching the ON
CONFLICT specification

+TRUNCATE temporal_rng;
+INSERT INTO temporal_rng (id, valid_at) VALUES ('[1,2)',
daterange('2000-01-01', '2010-01-01'));
+-- with a conflict
+INSERT INTO temporal_rng (id, valid_at) VALUES ('[1,2)',
daterange('2005-01-01', '2006-01-01')) ON CONFLICT ON CONSTRAINT
temporal_rng_pk DO NOTHING;

Re: SQL:2011 application time

[Paul Jungwirth]

On 3/24/24 00:38, Peter Eisentraut wrote:> I have committed the patches
v33-0001-Add-temporal-FOREIGN-KEYs.patch and v33-0002-Support-multiranges-in-temporal-FKs.patch 
(together).


Hi Hackers,

I found some problems with temporal primary keys and the idea of uniqueness, especially around the 
indisunique column. Here are some small fixes and a proposal for a larger fix, which I think we need 
but I'd like some feedback on.


The first patch fixes problems with ON CONFLICT DO NOTHING/UPDATE.

DO NOTHING fails because it doesn't expect a non-btree unique index. It's fine to make it accept a 
temporal PRIMARY KEY/UNIQUE index though (i.e. an index with both indisunique and indisexclusion).
This is no different than an exclusion constraint. So I skip BuildSpeculativeIndexInfo for WITHOUT 
OVERLAPS indexes. (Incidentally, AFAICT ii_UniqueOps is never used, only ii_UniqueProcs. Right?)


We should still forbid temporally-unique indexes for ON CONFLICT DO UPDATE, since there may be more 
than one row that conflicts. Ideally we would find and update all the conflicting rows, but we don't 
now, and we don't want to update just one:


postgres=# create table t (id int4range, valid_at daterange, name text, constraint tpk primary 
key (id, valid_at without overlaps));

CREATE TABLE
postgres=# insert into t values ('[1,2)', '[2000-01-01,2001-01-01)', 'a'), ('[1,2)', 
'[2001-01-01,2002-01-01)', 'b');

INSERT 0 2
postgres=# insert into t values ('[1,2)', '[2000-01-01,2002-01-01)', 'c') on conflict (id, 
valid_at) do update set name = excluded.name;

INSERT 0 1
postgres=# select * from t;
  id   |valid_at | name
---+-+--
 [1,2) | [2001-01-01,2002-01-01) | b
 [1,2) | [2000-01-01,2001-01-01) | c
(2 rows)

So I also added code to prevent that. This is just preserving the old behavior for exclusion 
constraints, which was bypassed because of indisunique. All this is in the first patch.


That got me thinking about indisunique and where else it could cause problems. Perhaps there are 
other places that assume only b-trees are unique. I couldn't find anywhere that just gives an error 
like ON CONFLICT, but I can imagine more subtle problems.


A temporal PRIMARY KEY or UNIQUE constraint is unique in at least three ways: It is *metaphorically* 
unique: the conceit is that the scalar part is unique at every moment in time. You may have id 5 in 
your table more than once, as long as the records' application times don't overlap.


And it is *officially* unique: the standard calls these constraints unique. I think it is correct 
for us to report them as unique in pg_index.


But is it *literally* unique? Well two identical keys, e.g. (5, '[Jan24,Mar24)') and (5, 
'[Jan24,Mar24)'), do have overlapping ranges, so the second is excluded. Normally a temporal unique 
index is *more* restrictive than a standard one, since it forbids other values too (e.g. (5, 
'[Jan24,Feb24)')). But sadly there is one exception: the ranges in these keys do not overlap: (5, 
'empty'), (5, 'empty'). With ranges/multiranges, `'empty' && x` is false for all x. You can add that 
key as many times as you like, despite a PK/UQ constraint:


postgres=# insert into t values
('[1,2)', 'empty', 'foo'),
('[1,2)', 'empty', 'bar');
INSERT 0 2
postgres=# select * from t;
  id   | valid_at | name
---+--+--
 [1,2) | empty| foo
 [1,2) | empty| bar
(2 rows)

Cases like this shouldn't actually happen for temporal tables, since empty is not a meaningful 
value. An UPDATE/DELETE FOR PORTION OF would never cause an empty. But we should still make sure 
they don't cause problems.


One place we should avoid temporally-unique indexes is REPLICA IDENTITY. Fortunately we already do 
that, but patch 2 adds a test to keep it that way.


Uniqueness is an important property to the planner, too.

We consider indisunique often for estimates, where it needn't be 100% true. Even if there are 
nullable columns or a non-indimmediate index, it still gives useful stats. Duplicates from 'empty' 
shouldn't cause any new problems there.


In proof code we must be more careful. Patch 3 updates relation_has_unique_index_ext and 
rel_supports_distinctness to disqualify WITHOUT OVERLAPS indexes. Maybe that's more cautious than 
needed, but better safe than sorry. This patch has no new test though. I had trouble writing SQL 
that was wrong before its change. I'd be happy for help here!


Another problem is GROUP BY and functional dependencies. This is wrong:

postgres=# create table a (id int4range, valid_at daterange, name text, constraint apk primary 
key (id, valid_at without overlaps));

CREATE TABLE
postgres=# insert into a values ('[1,2)', 'empty', 'foo'), ('[1,2)', 
'empty', 'bar');
INSERT 0 2
postgres=# select * from a group by id, valid_at;
  id   | valid_at | name
---+--+--
 [1,2) | 

Re: SQL:2011 application time

[jian he]

On Sun, Mar 24, 2024 at 1:42 AM Paul Jungwirth
 wrote:
>
> v33 attached with minor changes.
>
> Okay, added those tests too. Thanks!
>
> Rebased to 697f8d266c.
>


hi.
minor issues I found in v33-0003.
there are 29 of {check_amproc_signature?.*false}
only one {check_amproc_signature(procform->amproc, opcintype, true}
is this refactoring really worth it?

We also need to refactor gistadjustmembers?


+  
+   intersect
+   computes intersection with FOR PORTION OF
+bounds
+   13
+  
+  
+   without_portion
+   computes remaining duration(s) outside
+   FOR PORTION OF bounds
+   14
+  
needs to add "(optional)".


+
+Datum
+my_range_intersect(PG_FUNCTION_ARGS)
+{
+RangeType  *r1 = PG_GETARG_RANGE_P(0);
+RangeType  *r2 = PG_GETARG_RANGE_P(1);
+TypeCacheEntry *typcache;
+
+/* Different types should be prevented by ANYRANGE matching rules */
+if (RangeTypeGetOid(r1) != RangeTypeGetOid(r2))
   elog(ERROR, "range
types do not match");
+
+typcache = range_get_typcache(fcinfo, RangeTypeGetOid(r1));
+
+PG_RETURN_RANGE_P(range_intersect_internal(typcache, r1, r2));
+}
+
the elog, ERROR indentation is wrong?


+/*
+ * range_without_portion_internal - Sets outputs and outputn to the ranges
+ * remaining and their count (respectively) after subtracting r2 from r1.
+ * The array should never contain empty ranges.
+ * The outputs will be ordered. We expect that outputs is an array of
+ * RangeType pointers, already allocated with two slots.
+ */
+void
+range_without_portion_internal(TypeCacheEntry *typcache, RangeType *r1,
+   RangeType *r2, RangeType **outputs, int *outputn)
the comments need to be refactored?
there is nothing related to "slot"?
not sure the "array" description is right.
(my understanding is compute rangetype r1 and r2, and save the result to
RangeType **outputs.


select proisstrict, proname from pg_proc where proname =
'range_without_portion';
range_without_portion is strict.
but
select range_without_portion(NULL::int4range, int4range(11, 20,'[]'));
return zero rows.
Is this the expected behavior?


0003 seems simple enough.
but it's more related to "for portion of".
not sure we can push 0003 into v17.

Re: SQL:2011 application time

[Peter Eisentraut]

On 23.03.24 18:42, Paul Jungwirth wrote:
Now this is a long chain of reasoning to say rangetypes are safe. I 
added a comment. Note it doesn't apply to arbitrary types, so if we 
support those eventually we should just require a recheck always, or 
alternately use equals, not containedby. (That would require storing 
equals somewhere. It could go in ffeqop, but that feels like a footgun 
since pfeqop and ppeqop need overlaps.)


Ok, this explanation is good enough for now.  I have committed the 
patches v33-0001-Add-temporal-FOREIGN-KEYs.patch and 
v33-0002-Support-multiranges-in-temporal-FKs.patch (together).

Re: SQL:2011 application time

[jian he]

On Fri, Mar 22, 2024 at 11:49 PM Peter Eisentraut  wrote:
>
> On 22.03.24 01:35, Paul Jungwirth wrote:
> >  > 1. In ri_triggers.c ri_KeysEqual, you swap the order of arguments to
> > ri_AttributesEqual():
> >  >
> >  > -   if (!ri_AttributesEqual(riinfo->ff_eq_oprs[i],
> > RIAttType(rel, attnums[i]),
> >  > -   oldvalue, newvalue))
> >  > +   if (!ri_AttributesEqual(eq_opr, RIAttType(rel, attnums[i]),
> >  > +   newvalue, oldvalue))
> >  >
> >  > But the declared arguments of ri_AttributesEqual() are oldvalue and
> > newvalue, so passing them
> >  > backwards is really confusing.  And the change does matter in the tests.
> >  >
> >  > Can we organize this better?
> >
> > I renamed the params and actually the whole function. All it's doing is
> > execute `oldvalue op newvalue`, casting if necessary. So I changed it to
> > ri_CompareWithCast and added some documentation. In an earlier version
> > of this patch I had a separate function for the PERIOD comparison, but
> > it's just doing the same thing, so I think the best thing is to give the
> > function a more accurate name and use it.
>
> Ok, I see now, and the new explanation is better.
>
> But after reading the comment in the function about collations, I think
> there could be trouble.  As long as we are only comparing for equality
> (and we don't support nondeterministic global collations), then we can
> use any collation to compare for equality.  But if we are doing
> contained-by, then the collation does matter, so we would need to get
> the actual collation somehow.  So as written, this might not always work
> correctly.
>
> I think it would be safer for now if we just kept using the equality
> operation even for temporal foreign keys.  If we did that, then in the
> case that you update a key to a new value that is contained by the old
> value, this function would say "not equal" and fire all the checks, even
> though it wouldn't need to.  This is kind of similar to the "false
> negatives" that the comment already talks about.
>
> What do you think?
>

we don't need to worry about primary key and foreign key with
different collation.
because it will be error out as incompatible data type,
foreign key constraint will not be created.

if there are the same collation, when we build the query string, we
don't need to worry about collation.
because at runtime, the operator associated oprcode
will fetch collation information later.

main operator and the main oprcode related to this patch(0001, 0002) are:
range_contained_by_multirange
range_eq
range_overlaps
range_contained_by
the first 3 functions will fetch collation information within range_cmp_bounds.
range_contained_by will fetch collation information in
range_contains_elem_internal.

demo:
CREATE COLLATION case_insensitive (provider = icu, locale =
'und-u-ks-level2', deterministic = false);
DROP TABLE IF exists temporal_fk_rng2rng;
DROP TABLE IF exists temporal_rng;
DROP TYPE textrange_case_insensitive;
create type textrange_case_insensitive as range(subtype=text,
collation=case_insensitive);
CREATE TABLE temporal_rng (id int4range, valid_at textrange_case_insensitive);
ALTER TABLE temporal_rng
ADD CONSTRAINT temporal_rng_pk
PRIMARY KEY (id, valid_at WITHOUT OVERLAPS);
CREATE TABLE temporal_fk_rng2rng (
id int4range,
valid_at  textrange_case_insensitive,
parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_fk2 FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng (id, PERIOD valid_at)
);
INSERT INTO temporal_rng (id, valid_at) VALUES ('[1,2)',
textrange_case_insensitive('c', 'h','[]'));

--fail
INSERT INTO temporal_fk_rng2rng (id, valid_at, parent_id)
VALUES ('[1,2)', textrange_case_insensitive('B', 'B','[]'), '[1,2)');

--fail.
INSERT INTO temporal_fk_rng2rng (id, valid_at, parent_id)
VALUES ('[1,2)', textrange_case_insensitive('a', 'F','[]'), '[1,2)');

--fail.
INSERT INTO temporal_fk_rng2rng (id, valid_at, parent_id)
VALUES ('[1,2)', textrange_case_insensitive('e', 'Z','[]'), '[1,2)');

--ok
INSERT INTO temporal_fk_rng2rng (id, valid_at, parent_id)
VALUES ('[1,2)', textrange_case_insensitive('d', 'F','[]'), '[1,2)');

Re: SQL:2011 application time

[Peter Eisentraut]

On 22.03.24 01:35, Paul Jungwirth wrote:
 > 1. In ri_triggers.c ri_KeysEqual, you swap the order of arguments to 
ri_AttributesEqual():

 >
 > -   if (!ri_AttributesEqual(riinfo->ff_eq_oprs[i], 
RIAttType(rel, attnums[i]),

 > -   oldvalue, newvalue))
 > +   if (!ri_AttributesEqual(eq_opr, RIAttType(rel, attnums[i]),
 > +   newvalue, oldvalue))
 >
 > But the declared arguments of ri_AttributesEqual() are oldvalue and 
newvalue, so passing them

 > backwards is really confusing.  And the change does matter in the tests.
 >
 > Can we organize this better?

I renamed the params and actually the whole function. All it's doing is 
execute `oldvalue op newvalue`, casting if necessary. So I changed it to 
ri_CompareWithCast and added some documentation. In an earlier version 
of this patch I had a separate function for the PERIOD comparison, but 
it's just doing the same thing, so I think the best thing is to give the 
function a more accurate name and use it.


Ok, I see now, and the new explanation is better.

But after reading the comment in the function about collations, I think 
there could be trouble.  As long as we are only comparing for equality 
(and we don't support nondeterministic global collations), then we can 
use any collation to compare for equality.  But if we are doing 
contained-by, then the collation does matter, so we would need to get 
the actual collation somehow.  So as written, this might not always work 
correctly.


I think it would be safer for now if we just kept using the equality 
operation even for temporal foreign keys.  If we did that, then in the 
case that you update a key to a new value that is contained by the old 
value, this function would say "not equal" and fire all the checks, even 
though it wouldn't need to.  This is kind of similar to the "false 
negatives" that the comment already talks about.


What do you think?

Re: SQL:2011 application time

[jian he]

On Fri, Mar 22, 2024 at 8:35 AM Paul Jungwirth
 wrote:
>
> Your patch had a lot of other noisy changes, e.g.
> whitespace and reordering lines. If there are other things you intended to 
> add to the tests, can you
> describe them?

i think on update restrict, on delete restrict cannot be deferred,
even if you set it DEFERRABLE INITIALLY DEFERRED.
based on this idea, I made minor change on
v32-0002-Support-multiranges-in-temporal-FKs.patch

other than that, v32, 0002 looks good.


v32-0001-minor-refactor-temporal-FKs-with-multiranges-.no-cfbot
Description: Binary data

Re: SQL:2011 application time

[Peter Eisentraut]

On 20.03.24 17:21, Paul Jungwirth wrote:

On 3/20/24 03:55, jian he wrote:

hi.
minor cosmetic issues, other than that, looks good.

*pk_period = (indexStruct->indisexclusion);
to
*pk_period = indexStruct->indisexclusion;

... >
if (with_period && !fkconstraint->fk_with_period)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("foreign key uses PERIOD on the referenced table but not the
referencing table")));


Both included in the new patches here.

Rebased to a0390f6ca6.


Two more questions:

1. In ri_triggers.c ri_KeysEqual, you swap the order of arguments to 
ri_AttributesEqual():


-   if (!ri_AttributesEqual(riinfo->ff_eq_oprs[i], 
RIAttType(rel, attnums[i]),

-   oldvalue, newvalue))
+   if (!ri_AttributesEqual(eq_opr, RIAttType(rel, attnums[i]),
+   newvalue, oldvalue))

But the declared arguments of ri_AttributesEqual() are oldvalue and 
newvalue, so passing them backwards is really confusing.  And the change 
does matter in the tests.


Can we organize this better?

2. There are some tests that error with

ERROR:  only b-tree indexes are supported for non-PERIOD foreign keys

But this is an elog() error, so should not normally be visible.  I 
suspect some other error should really show here, and the order of 
checks is a bit wrong or something?

Re: SQL:2011 application time

[jian he]

with foreign key "no action",
in a transaction, we can first insert foreign key data, then primary key data.
also the update/delete can fail at the end of transaction.

based on [1] explanation about the difference between "no action" and
"restrict".
I only refactor the v31-0002-Support-multiranges-in-temporal-FKs.patch test.


[1 
https://stackoverflow.com/questions/14921668/difference-between-restrict-and-no-action


v31-0001-refactor-temporal-FKs-with-multiranges-regressio.no-cfbot
Description: Binary data

Re: SQL:2011 application time

[jian he]

hi.
minor cosmetic issues, other than that, looks good.

*pk_period = (indexStruct->indisexclusion);
to
*pk_period = indexStruct->indisexclusion;


if (with_period)
{
if (!fkconstraint->fk_with_period)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("foreign key uses PERIOD on the referenced table but not the
referencing table")));
}

change to

if (with_period && !fkconstraint->fk_with_period)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("foreign key uses PERIOD on the referenced table but not the
referencing table")));

Re: SQL:2011 application time

[Peter Eisentraut]

On 16.03.24 22:37, Paul A Jungwirth wrote:

Here is a new patch series addressing the last few feedback emails
from Peter & Jian He. It mostly focuses on the FKs patch, trying to
get it really ready to commit,


I have committed the test changes (range and date format etc.).

The FOREIGN KEY patch looks okay to me now.  Maybe check if any of the 
subsequent comments from jian should be applied.



  > I'm not sure how else to do it. The issue is that `range_agg` returns
  > a multirange, so the result
  > type doesn't match the inputs. But other types will likely have the
  > same problem: to combine boxes
  > you may need a multibox. The combine mdranges you may need a
  > multimdrange.

Can we just hardcode the use of range_agg for this release?  Might be
easier.  I don't see all this generality being useful in the near future.


Okay, I've hard-coded range_agg in the main patch and separated the
support for multirange/etc in the next two patches. But there isn't
much code there (mostly tests and docs). Since we can't hard-code the
*operators*, most of the infrastructure is already there not to
hard-code the aggregate function. Supporting multiranges is already a
nice improvement. E.g. it should cut down on disk usage when a record
gets updated frequently. Supporting arbitrary types also seems very
powerful, and we already do that for PKs.


I think we could also handle multiranges in a hardcoded way?  Ranges and 
multiranges are hardcoded concepts anyway.  It's just when we move to 
arbitrary types supporting containment, then it gets a bit more complicated.


What would a patch that adds just multiranges on the FK side, but 
without the full pluggable gist support, look like?



I don't see any drawbacks from supporting inferred REFERENCES with
temporal tables, so my vote is to break from the standard here, and
*not* apply that follow-up patch. Should I add some docs about that?
Also skipping the patch will cause some annoying merge conflicts, so
let me know if that's what you choose and I'll handle them right away.


I agree we can allow this.

Re: SQL:2011 application time

[jian he]

On Tue, Mar 19, 2024 at 6:49 AM Paul Jungwirth
 wrote:
>
> Rebased to 846311051e.
>

Hi, I just found out some minor issues.

+ * types matching the PERIOD element. periodprocoid is a GiST support
function to
+ * aggregate multiple PERIOD element values into a single value
+ * (whose return type need not match its inputs,
+ * e.g. many ranges can be aggregated into a multirange).
  * And aggedperiodoperoid is also a ContainedBy operator,
- * but one whose rhs is anymultirange.
+ * but one whose rhs matches the type returned by aggedperiodoperoid.
  * That way foreign keys can compare fkattr <@ range_agg(pkattr).
  */
 void
-FindFKPeriodOpers(Oid opclass,
-  Oid *periodoperoid,
-  Oid *aggedperiodoperoid)
+FindFKPeriodOpersAndProcs(Oid opclass,
+  Oid *periodoperoid,
+  Oid *aggedperiodoperoid,
+  Oid *periodprocoid)

I think, aggedperiodoperoid is more descriptive than periodprocoid, in
0005, you don't need to rename it.
aslo do we need to squash v29 0001 to 0005 together?

--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1167,7 +1167,8 @@ WITH ( MODULUS numeric_literal, REM
   column(s) of some row of the referenced table.  If the refcolumn list is omitted, the
   primary key of the reftable
-  is used.  Otherwise, the refcolumn
+  is used (omitting any part declared with WITHOUT
OVERLAPS).
+  Otherwise, the refcolumn
   list must refer to the columns of a non-deferrable unique or primary key
   constraint or be the columns of a non-partial unique index.
  
I think this does not express that
foreign key is PERIOD, then the last column of refcolumn must specify PERIOD?

+ 
+  If the last column is marked with PERIOD,
+  it is treated in a special way.
+  While the non-PERIOD columns are compared for equality
+  (and there must be at least one of them),
+  the PERIOD column is not.
+  Instead the constraint is considered satisfied
+  if the referenced table has matching records
+  (based on the non-PERIOD parts of the key)
+  whose combined PERIOD values completely cover
+  the referencing record's.
+  In other words, the reference must have a referent for its
entire duration.
+  This column must be a column with a range type.
+  In addition the referenced table must have a primary key
+  or unique constraint declared with WITHOUT PORTION.
+ 
you forgot to change  WITHOUT PORTION to
WITHOUT OVERLAPS


Oid pf_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (PK = FK) */
Oid pp_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (PK = PK) */
Oid ff_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (FK = FK) */
in struct RI_ConstraintInfo, these comments need to be updated?

Re: SQL:2011 application time

[jian he]

one more minor issue related to error reporting.
I've only applied v28, 0001 to 0005.

-- (parent_id, valid_at) REFERENCES [implicit]
-- FOREIGN KEY part should specify PERIOD
CREATE TABLE temporal_fk_rng2rng (
id int4range,
valid_at daterange,
parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, valid_at)
REFERENCES temporal_rng
);
ERROR:  number of referencing and referenced columns for foreign key disagree

-- (parent_id, PERIOD valid_at) REFERENCES (id)
CREATE TABLE temporal_fk_rng2rng (
id int4range,
valid_at daterange,
parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng (id)
);
ERROR:  foreign key uses PERIOD on the referencing table but not the
referenced table

these error messages seem somehow inconsistent with the comments above?


+ else
+ {
+ /*
+ * Check it's a btree; currently this can never fail since no other
+ * index AMs support unique indexes.  If we ever did have other types
+ * of unique indexes, we'd need a way to determine which operator
+ * strategy number is equality.  (Is it reasonable to insist that
+ * every such index AM use btree's number for equality?)
+ */
+ if (amid != BTREE_AM_OID)
+ elog(ERROR, "only b-tree indexes are supported for foreign keys");
+ eqstrategy = BTEqualStrategyNumber;
+ }

the comments say never fail.
but it actually failed. see:

+-- (parent_id) REFERENCES [implicit]
+-- This finds the PK (omitting the WITHOUT OVERLAPS element),
+-- but it's not a b-tree index, so it fails anyway.
+-- Anyway it must fail because the two sides have a different
definition of "unique".
+CREATE TABLE temporal_fk_rng2rng (
+ id int4range,
+ valid_at daterange,
+ parent_id int4range,
+ CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
+ CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id)
+ REFERENCES temporal_rng
+);
+ERROR:  only b-tree indexes are supported for foreign keys

because in transformFkeyGetPrimaryKey.
we have `if (indexStruct->indisexclusion && i == indexStruct->indnatts - 1)`
we have pk_with_period, fk_with_period in Constraint struct.

maybe we can add a bool argument to transformFkeyGetPrimaryKey
indicate, this primary key is a conperiod constraint.
then we can check condition: the primary key is a conperiod constraint
and fk_with_period or is pk_with_period is false

I've made a patch to make these error reporting more accurate.
you can further refine it.


v28-0001-refactor-transformFkeyGetPrimaryKey-for-bette.no-cfbot
Description: Binary data

Re: SQL:2011 application time

[jian he]

Hi, minor issues from 1 to 0005.
+  
+   referencedagg
+   aggregates referenced rows' WITHOUT OVERLAPS
+part
+   13
+  
comparing with surrounding items, maybe need to add `(optional)`?
I think the explanation is not good as explained in referencedagg entry below:
  
   An aggregate function. Given values of this opclass,
   it returns a value combining them all. The return value
   need not be the same type as the input, but it must be a
   type that can appear on the right hand side of the "contained by"
   operator. For example the built-in range_ops
   opclass uses range_agg here, so that foreign
   keys can check fkperiod @> range_agg(pkperiod).
  


+  In other words, the reference must have a referent for its
entire duration.
+  This column must be a column with a range type.
+  In addition the referenced table must have a primary key
+  or unique constraint declared with WITHOUT PORTION.
+ 
seems you missed replacing this one.


in v28-0002, the function name is FindFKPeriodOpers,
then in v28-0005 rename it to FindFKPeriodOpersAndProcs?
renaming the function name in a set of patches seems not a good idea?


+  
+   This is used for temporal foreign key constraints.
+   If you omit this support function, your type cannot be used
+   as the PERIOD part of a foreign key.
+  
in v28-0004, I think here "your type"  should change to "your opclass"?

+bool
+check_amproc_is_aggregate(Oid funcid)
+{
+ bool result;
+ HeapTuple tp;
+ Form_pg_proc procform;
+
+ tp = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+ if (!HeapTupleIsValid(tp))
+ elog(ERROR, "cache lookup failed for function %u", funcid);
+ procform = (Form_pg_proc) GETSTRUCT(tp);
+ result = procform->prokind == 'a';
+ ReleaseSysCache(tp);
+ return result;
+}
maybe
`
change procform->prokind == 'a';
`
to
`
procform->prokind == PROKIND_AGGREGATE;
`
or we can put the whole function to cache/lsyscache.c
name it just as proc_is_aggregate.


-  Added pg_dump support.
- Show the correct syntax in psql \d output for foreign keys.
in 28-0002, seems there is no work to correspond to these 2 items in
the commit message?


@@ -12335,7 +12448,8 @@ validateForeignKeyConstraint(char *conname,
  Relation rel,
  Relation pkrel,
  Oid pkindOid,
- Oid constraintOid)
+ Oid constraintOid,
+ bool temporal)
do you need to change the last argument of this function to "is_period"?


+ sprintf(paramname, "$%d", riinfo->nkeys);
+ sprintf(paramname, "$%d", riinfo->nkeys);
do you think it worth the trouble to change to snprintf, I found
related post on [1].

[1] https://stackoverflow.com/a/7316500/15603477

Re: SQL:2011 application time

[jian he]

in GetOperatorFromWellKnownStrategy:
*strat = GistTranslateStratnum(opclass, instrat);
if (*strat == InvalidStrategy)
{
HeapTuple tuple;
tuple = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for operator class %u", opclass);
ereport(ERROR,
errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg(errstr, format_type_be(opcintype)),
errdetail("Could not translate strategy number %d for operator class
\"%s\" for access method \"%s\".",
  instrat, NameStr(((Form_pg_opclass) GETSTRUCT(tuple))->opcname), "gist"));
ReleaseSysCache(tuple);
}

last `ReleaseSysCache(tuple);` is unreachable?


@@ -118,12 +120,17 @@ typedef struct RI_ConstraintInfo
  int16 confdelsetcols[RI_MAX_NUMKEYS]; /* attnums of cols to set on
  * delete */
  char confmatchtype; /* foreign key's match type */
+ bool temporal; /* if the foreign key is temporal */
  int nkeys; /* number of key columns */
  int16 pk_attnums[RI_MAX_NUMKEYS]; /* attnums of referenced cols */
  int16 fk_attnums[RI_MAX_NUMKEYS]; /* attnums of referencing cols */
  Oid pf_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (PK = FK) */
  Oid pp_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (PK = PK) */
  Oid ff_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (FK = FK) */
+ Oid period_contained_by_oper; /* operator for PERIOD SQL */
+ Oid agged_period_contained_by_oper; /* operator for PERIOD SQL */
+ Oid period_referenced_agg_proc; /* proc for PERIOD SQL */
+ Oid period_referenced_agg_rettype; /* rettype for previous */

the comment seems not clear to me. Here is my understanding about it:
period_contained_by_oper is the operator where a single period/range
contained by a single period/range.
agged_period_contained_by_oper is the operator oid where a period
contained by a bound of periods
period_referenced_agg_proc is the oprcode of the agged_period_contained_by_oper.
period_referenced_agg_rettype is the function
period_referenced_agg_proc returning data type.

Re: SQL:2011 application time

[jian he]

On Mon, Mar 11, 2024 at 3:46 PM Peter Eisentraut  wrote:
>
> A few general comments on the tests:
>
> - In the INSERT commands, specify the column names explicitly.  This
> makes the tests easier to read (especially since the column order
> between the PK and the FK table is sometimes different).
>
> - Let's try to make it so that the inserted literals match the values
> shown in the various error messages, so it's easier to match them up.
> So, change the int4range literals to half-open notation.  And also maybe
> change the date output format to ISO.
>
maybe just change the tsrange type to daterange, then the dot out file
will be far less verbose.

minor issues while reviewing v27, 0001 to 0004.
transformFkeyGetPrimaryKey comments need to update,
since bool pk_period also returned.

+/*
+ * FindFKComparisonOperators -
+ *
+ * Gets the operators for pfeqopOut, ppeqopOut, and ffeqopOut.
+ * Sets old_check_ok if we can avoid re-validating the constraint.
+ * Sets old_pfeqop_item to the old pfeqop values.
+ */
+static void
+FindFKComparisonOperators(Constraint *fkconstraint,
+  AlteredTableInfo *tab,
+  int i,
+  int16 *fkattnum,
+  bool *old_check_ok,
+  ListCell **old_pfeqop_item,
+  Oid pktype, Oid fktype, Oid opclass,
+  Oid *pfeqopOut, Oid *ppeqopOut, Oid *ffeqopOut)

I think the above comments is
`Sets the operators for pfeqopOut, ppeqopOut, and ffeqopOut.`.


+ if (is_temporal)
+ {
+ if (!fkconstraint->fk_with_period)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FOREIGN_KEY),
+ errmsg("foreign key uses PERIOD on the referenced table but not the
referencing table")));
+ }
can be
if (is_temporal && !fkconstraint->fk_with_period)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("foreign key uses PERIOD on the referenced table but not the
referencing table")));

+
+ if (is_temporal)
+ {
+ if (!fkconstraint->pk_with_period)
+ /* Since we got pk_attrs, one should be a period. */
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FOREIGN_KEY),
+ errmsg("foreign key uses PERIOD on the referencing table but not the
referenced table")));
+ }
can be
if (is_temporal && !fkconstraint->pk_with_period)
/* Since we got pk_attrs, one should be a period. */
ereport(ERROR,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("foreign key uses PERIOD on the referencing table but not the
referenced table")));

refactor decompile_column_index_array seems unnecessary.
Peter already mentioned it at [1], I have tried to fix it at [2].


@@ -12141,7 +12245,8 @@ transformFkeyGetPrimaryKey(Relation pkrel, Oid
*indexOid,
  /*
  * Now build the list of PK attributes from the indkey definition (we
- * assume a primary key cannot have expressional elements)
+ * assume a primary key cannot have expressional elements, unless it
+ * has a PERIOD)
  */
  *attnamelist = NIL;
  for (i = 0; i < indexStruct->indnkeyatts; i++)
@@ -12155,6 +12260,8 @@ transformFkeyGetPrimaryKey(Relation pkrel, Oid
*indexOid,
makeString(pstrdup(NameStr(*attnumAttName(pkrel, pkattno);
  }
+ *pk_period = (indexStruct->indisexclusion);

I  don't understand the "expression elements" in the comments, most of
the tests case is like
`
PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
`
+ *pk_period = (indexStruct->indisexclusion);
can be
`+ *pk_period = indexStruct->indisexclusion;`


[1] https://postgr.es/m/7be8724a-5c25-46d7-8325-1bd8be6fa...@eisentraut.org
[2] 
https://postgr.es/m/CACJufxHVg65raNhG2zBwXgjrD6jqace4NZbePyMhP8-_Q=i...@mail.gmail.com

Re: SQL:2011 application time

[jian he]

+ 
+  If the last column is marked with PERIOD,
+  it is treated in a special way.
+  While the non-PERIOD columns are treated normally
+  (and there must be at least one of them),
+  the PERIOD column is not compared for equality.
+  Instead the constraint is considered satisfied
+  if the referenced table has matching records
+  (based on the non-PERIOD parts of the key)
+  whose combined PERIOD values completely cover
+  the referencing record's.
+  In other words, the reference must have a referent for its
entire duration.
+  Normally this column would be a range or multirange type,
+  although any type whose GiST opclass has a "contained by" operator
+  and a referenced_agg support function is allowed.
+  (See .)
+  In addition the referenced table must have a primary key
+  or unique constraint declared with WITHOUT PORTION.
+ 

typo "referenced_agg", in the gist-extensibility.html page is "referencedagg"
WITHOUT PORTION should be WITHOUT OVERLAPS

+  While the non-PERIOD columns are treated normally
+  (and there must be at least one of them),
+  the PERIOD column is not compared for equality.
the above sentence didn't say what is "normally"?
maybe we can do the following:
+  While the non-PERIOD columns are treated
+ normally for equality
+  (and there must be at least one of them),
+  the PERIOD column is not compared for equality.



+
+Datum
+my_range_agg_transfn(PG_FUNCTION_ARGS)
+{
+MemoryContextaggContext;
+Oid  rngtypoid;
+ArrayBuildState *state;
+
+if (!AggCheckCallContext(fcinfo, aggContext))
+elog(ERROR, "range_agg_transfn called in non-aggregate context");
+
+rngtypoid = get_fn_expr_argtype(fcinfo-flinfo, 1);
+if (!type_is_range(rngtypoid))
+elog(ERROR, "range_agg must be called with a range");
+
+if (PG_ARGISNULL(0))
+state = initArrayResult(rngtypoid, aggContext, false);
+else
+state = (ArrayBuildState *) PG_GETARG_POINTER(0);
+
+/* skip NULLs */
+if (!PG_ARGISNULL(1))
+accumArrayResult(state, PG_GETARG_DATUM(1), false, rngtypoid,
aggContext);
+
+PG_RETURN_POINTER(state);
+}
+
+Datum
+my_range_agg_finalfn(PG_FUNCTION_ARGS)
+{
+MemoryContextaggContext;
+Oid  mltrngtypoid;
+TypeCacheEntry  *typcache;
+ArrayBuildState *state;
+int32range_count;
+RangeType  **ranges;
+int  i;
+
+if (!AggCheckCallContext(fcinfo, aggContext))
+elog(ERROR, "range_agg_finalfn called in non-aggregate context");
+
+state = PG_ARGISNULL(0) ? NULL : (ArrayBuildState *) PG_GETARG_POINTER(0);
+if (state == NULL)
+/* This shouldn't be possible, but just in case */
+PG_RETURN_NULL();
+
+/* Also return NULL if we had zero inputs, like other aggregates */
+range_count = state-nelems;
+if (range_count == 0)
+PG_RETURN_NULL();
+
+mltrngtypoid = get_fn_expr_rettype(fcinfo-flinfo);
+typcache = multirange_get_typcache(fcinfo, mltrngtypoid);
+
+ranges = palloc0(range_count * sizeof(RangeType *));
+for (i = 0; i  range_count; i++)
+ranges[i] = DatumGetRangeTypeP(state-dvalues[i]);
+
+PG_RETURN_MULTIRANGE_P(make_multirange(mltrngtypoid,
typcache-rngtype, range_count, ranges));
+}

my_range_agg_transfn error message is inconsistent?
 `elog(ERROR, "range_agg_transfn called in non-aggregate context");`
`elog(ERROR, "range_agg must be called with a range");`
maybe just `my_range_agg_transfn`, instead of mention
{range_agg_transfn|range_agg}
similarly my_range_agg_finalfn error is also inconsistent.

my_range_agg_finalfn need  `type_is_multirange(mltrngtypoid)`?

Re: SQL:2011 application time

[Peter Eisentraut]

On 01.03.24 22:56, Paul Jungwirth wrote:

On 3/1/24 12:38, Paul Jungwirth wrote:

On 2/29/24 13:16, Paul Jungwirth wrote:
Here is a v26 patch series to fix a cfbot failure in sepgsql. Rebased 
to 655dc31046.


v27 attached, fixing some cfbot failures from 
headerscheck+cpluspluscheck. Sorry for the noise!


I had committed v27-0001-Rename-conwithoutoverlaps-to-conperiod.patch a 
little while ago.


I have reviewed v27-0002 through 0004 now.  I have one semantic question 
below, and there are a few places where more clarification of the 
interfaces could help.  Other than that, I think this is pretty good.


Attached is a small patch that changes the PERIOD keyword to unreserved 
for this patch.  You had said earlier that this didn't work for you. 
The attached patch works for me when applied on top of 0003.



* v27-0002-Add-GiST-referencedagg-support-func.patch

You wrote:

> I'm not sure how else to do it. The issue is that `range_agg` returns 
> a multirange, so the result

> type doesn't match the inputs. But other types will likely have the
> same problem: to combine boxes
> you may need a multibox. The combine mdranges you may need a
> multimdrange.

Can we just hardcode the use of range_agg for this release?  Might be 
easier.  I don't see all this generality being useful in the near future.


> Btw that part changed a bit since v24 because as jian he pointed out, 
> our type system doesn't

> support anyrange inputs and an anyrange[] output. So I changed the
> support funcs to use SETOF.

I didn't see any SETOF stuff in the patch, or I didn't know where to look.

I'm not sure I follow all the details here.  So more explanations of any 
kind could be helpful.



* v27-0003-Refactor-FK-operator-lookup.patch

I suggest to skip this refactoring patch.  I don't think the way this is 
sliced up is all that great, and it doesn't actually help with the 
subsequent patches.



* v27-0004-Add-temporal-FOREIGN-KEYs.patch

- src/backend/catalog/pg_constraint.c

FindFKPeriodOpersAndProcs() could use a bit more top-level
documentation.  Where does the input opclass come from?  What are the
three output values?  What is the business with "symmetric types"?

- src/backend/commands/indexcmds.c

GetOperatorFromWellKnownStrategy() is apparently changed to accept
InvalidOid for rhstype, but the meaning of this is not explained in
the function header.  It's also not clear to me why an existing caller
is changed.  This should be explained more thoroughly.

- src/backend/commands/tablecmds.c

is_temporal and similar should be renamed to with_period or similar 
throughout this patch.


In transformFkeyGetPrimaryKey():

 * Now build the list of PK attributes from the indkey definition (we
-* assume a primary key cannot have expressional elements)
+* assume a primary key cannot have expressional elements, unless it
+* has a PERIOD)

I think the original statement is still true even with PERIOD.  The 
expressional elements refer to expression indexes.  I don't think we can 
have a PERIOD marker on an expression?


- src/backend/utils/adt/ri_triggers.c

Please remove the separate trigger functions for the period case.  They 
are the same as the non-period ones, so we don't need separate ones. 
The difference is handled lower in the call stack, which I think is a 
good setup.  Removing the separate functions also removes a lot of extra 
code in other parts of the patch.


- src/include/catalog/pg_constraint.h

Should also update catalogs.sgml accordingly.

- src/test/regress/expected/without_overlaps.out
- src/test/regress/sql/without_overlaps.sql

A few general comments on the tests:

- In the INSERT commands, specify the column names explicitly.  This 
makes the tests easier to read (especially since the column order 
between the PK and the FK table is sometimes different).


- Let's try to make it so that the inserted literals match the values 
shown in the various error messages, so it's easier to match them up. 
So, change the int4range literals to half-open notation.  And also maybe 
change the date output format to ISO.


- In various comments, instead of test FK "child", maybe use 
"referencing table"?  Instead of "parent", use "referenced table" (or 
primary key table).  When I read child and parent I was looking for 
inheritance.


- Consider truncating the test tables before each major block of tests 
and refilling them with fresh data.  So it's easier to eyeball the 
tests.  Otherwise, there is too much dependency on what earlier tests 
left behind.


A specific question:

In this test, a PERIOD marker on the referenced site is automatically 
inferred from the primary key:


+-- with inferred PK on the referenced table:
+CREATE TABLE temporal_fk_rng2rng (
+   id int4range,
+   valid_at tsrange,
+   parent_id int4range,
+   CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT 
OVERLAPS),
+   CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD 
valid_at)

+   

Re: SQL:2011 application time

[Paul Jungwirth]

On 2/13/24 21:00, jian he wrote:

Hi
more minor issues.

+ FindFKComparisonOperators(
+ fkconstraint, tab, i, fkattnum,
+ _check_ok, _pfeqop_item,
+ pktypoid[i], fktypoid[i], opclasses[i],
+ is_temporal, false,
+ [i], [i], [i]);
+ }
+ if (is_temporal) {
+ pkattnum[numpks] = pkperiodattnum;
+ pktypoid[numpks] = pkperiodtypoid;
+ fkattnum[numpks] = fkperiodattnum;
+ fktypoid[numpks] = fkperiodtypoid;

- pfeqop = get_opfamily_member(opfamily, opcintype, fktyped,
- eqstrategy);
- if (OidIsValid(pfeqop))
- {
- pfeqop_right = fktyped;
- ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
- eqstrategy);
- }
- else
- {
- /* keep compiler quiet */
- pfeqop_right = InvalidOid;
- ffeqop = InvalidOid;
- }
+ FindFKComparisonOperators(
+ fkconstraint, tab, numpks, fkattnum,
+ _check_ok, _pfeqop_item,
+ pkperiodtypoid, fkperiodtypoid, opclasses[numpks],
+ is_temporal, true,
+ [numpks], [numpks], [numpks]);
+ numfks += 1;
+ numpks += 1;
+ }

opening curly brace should be the next line,


Fixed in v25 (submitted in my other email).


also do you think it's
good idea to add following in the `if (is_temporal)` branch
`
Assert(OidIsValid(fkperiodtypoid) && OidIsValid(pkperiodtypoid));
Assert(OidIsValid(pkperiodattnum > 0 && fkperiodattnum > 0));
`

` if (is_temporal)` branch, you can set the FindFKComparisonOperators
10th argument (is_temporal)
to true, since you are already in the ` if (is_temporal)` branch.

maybe we need some extra comments on
`
+ numfks += 1;
+ numpks += 1;
`
since it might not be that evident?


That branch doesn't exist anymore. Same with the increments.


Do you think it's a good idea to list arguments line by line (with
good indentation) is good format? like:
FindFKComparisonOperators(fkconstraint,
tab,
i,
fkattnum,
_check_ok,
_pfeqop_item,
pktypoid[i],
fktypoid[i],
opclasses[i],
false,
false,
[i],
[i],
[i]);


There are places we do that, but most code I've seen tries to fill the line. I haven't followed that 
strictly here, but I'm trying to get better at doing what pg_indent wants.


Yours,

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

Re: SQL:2011 application time

[jian he]

Hi
more minor issues.

+ FindFKComparisonOperators(
+ fkconstraint, tab, i, fkattnum,
+ _check_ok, _pfeqop_item,
+ pktypoid[i], fktypoid[i], opclasses[i],
+ is_temporal, false,
+ [i], [i], [i]);
+ }
+ if (is_temporal) {
+ pkattnum[numpks] = pkperiodattnum;
+ pktypoid[numpks] = pkperiodtypoid;
+ fkattnum[numpks] = fkperiodattnum;
+ fktypoid[numpks] = fkperiodtypoid;

- pfeqop = get_opfamily_member(opfamily, opcintype, fktyped,
- eqstrategy);
- if (OidIsValid(pfeqop))
- {
- pfeqop_right = fktyped;
- ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
- eqstrategy);
- }
- else
- {
- /* keep compiler quiet */
- pfeqop_right = InvalidOid;
- ffeqop = InvalidOid;
- }
+ FindFKComparisonOperators(
+ fkconstraint, tab, numpks, fkattnum,
+ _check_ok, _pfeqop_item,
+ pkperiodtypoid, fkperiodtypoid, opclasses[numpks],
+ is_temporal, true,
+ [numpks], [numpks], [numpks]);
+ numfks += 1;
+ numpks += 1;
+ }

opening curly brace should be the next line, also do you think it's
good idea to add following in the `if (is_temporal)` branch
`
Assert(OidIsValid(fkperiodtypoid) && OidIsValid(pkperiodtypoid));
Assert(OidIsValid(pkperiodattnum > 0 && fkperiodattnum > 0));
`

` if (is_temporal)` branch, you can set the FindFKComparisonOperators
10th argument (is_temporal)
to true, since you are already in the ` if (is_temporal)` branch.

maybe we need some extra comments on
`
+ numfks += 1;
+ numpks += 1;
`
since it might not be that evident?

Do you think it's a good idea to list arguments line by line (with
good indentation) is good format? like:
FindFKComparisonOperators(fkconstraint,
tab,
i,
fkattnum,
_check_ok,
_pfeqop_item,
pktypoid[i],
fktypoid[i],
opclasses[i],
false,
false,
[i],
[i],
[i]);

Re: SQL:2011 application time

[Peter Eisentraut]

I have done a review of the temporal foreign key patches in this patch
series (0002 and 0003, v24).

The patch set needs a rebase across c85977d8fef.  I was able to do it
manually, but it's a bit tricky, so perhaps you can post a new set to
help future reviews.

(Also, the last (0007) patch has some compiler warnings and also
causes the pg_upgrade test to fail.  I didn't check this further, but
that's why the cfbot is all red.)

In summary, in principle, this all looks more or less correct to me.

As a general comment, we need to figure out the right terminology
"period" vs. "temporal", especially if we are going to commit these
features incrementally.  But I didn't look at this too hard here yet.


* v24-0002-Add-GiST-referencedagg-support-func.patch

Do we really need this level of generality?  Are there examples not
using ranges that would need a different aggregate function?  Maybe
something with geometry (points and lines)?  But it seems to me that
then we'd also need some equivalent to "without portion" support for
those types and a multirange equivalent (basically another gist
support function wrapped around the 0004 patch).


* v24-0003-Add-temporal-FOREIGN-KEYs.patch

- contrib/btree_gist/expected/without_overlaps.out
- contrib/btree_gist/sql/without_overlaps.sql

typo "exusts"


- doc/src/sgml/ref/create_table.sgml

This mentions FOR PORTION OF from a later patch.

It is not documented that SET NULL and SET DEFAULT are not supported,
even though that is added in a later patch.  (So this patch should say
that it's not supported, and then the later patch should remove that.)


- src/backend/commands/indexcmds.c

The changes to GetOperatorFromWellKnownStrategy() don't work for
message translations.  We had discussed a similar issue for this
function previously.  I think it's ok to leave the function as it was.
The additional context could be added with location pointers or
errcontext() maybe, but it doesn't seem that important for now.


- src/backend/commands/tablecmds.c

The changes in ATAddForeignKeyConstraint(), which are the meat of the
changes in this file, are very difficult to review in detail.  I tried
different git-diff options to get a sensible view, but it wasn't
helpful.  Do we need to do some separate refactoring here first?

The error message "action not supported for temporal foreign keys"
could be more detailed, mention the action.  Look for example how the
error for the generated columns is phrased.  (But note that for
generated columns, the actions are impossible to support, whereas here
it is just something not done yet.  So there should probably still be
different error codes.)


- src/backend/nodes/outfuncs.c
- src/backend/nodes/readfuncs.c

Perhaps you would like to review my patch 0001 in
,
which removes the custom out/read functions for the Constraint node.
Then you could get rid of these changes.


- src/backend/utils/adt/ri_triggers.c

The added #include "catalog/pg_range.h" doesn't appear to be used for
anything.

Maybe we can avoid the added #include "commands/tablecmds.h" by
putting the common function in some appropriate lower-level module.

typo "PEROID"

Renaming of ri_KeysEqual() to ri_KeysStable() doesn't improve clarity,
I think.  I think we can leave the old name and add a comment (as you
have done).  There is a general understanding around this feature set
that "equal" sometimes means "contained" or something like that.

The function ri_RangeAttributeNeedsCheck() could be documented better.
It's bit terse and unclear.  From the code, it looks like it is used
instead of row equality checks.  Maybe a different function name would
be suitable.

Various unnecessary reformatting in RI_FKey_check().

When assembling the SQL commands, you need to be very careful about
fully quoting and schema-qualifying everything.  See for example
ri_GenerateQual().

Have you checked that the generated queries can use indexes and have
suitable performance?  Do you have example execution plans maybe?


- src/backend/utils/adt/ruleutils.c

This seems ok in principle, but it's kind of weird that the new
argument of decompile_column_index_array() is called "withPeriod"
(which seems appropriate seeing what it does), but what we are passing
in is conwithoutoverlaps.  Maybe we need to reconsider the naming of
the constraint column?  Sorry, I made you change it from "contemporal"
or something, didn't I?  Maybe "conperiod" would cover both meanings
better?


- src/backend/utils/cache/lsyscache.c

get_func_name_and_namespace(): This function would at least need some
identifier quoting.  There is only one caller (lookupTRIOperAndProc),
so let's just put this code inline there; it's not worth a separate
global function.  (Also, you could use psprintf() here to simplify
palloc() + snprintf().)


- src/include/catalog/pg_constraint.h

You are changing in several comments "equality" to "comparison".  I

Re: SQL:2011 application time

[jian he]

On Mon, Jan 29, 2024 at 8:00 AM jian he  wrote:
>
> I fixed your tests, some of your tests can be simplified, (mainly
> primary key constraint is unnecessary for the failed tests)
> also your foreign key patch test table, temporal_rng is created at
> line 141, and we use it at around line 320.
> it's hard to get the definition of temporal_rng.  I drop the table
> and recreate it.
> So people can view the patch with tests more easily.
>
I've attached a new patch that further simplified the tests. (scope
v24 patch's 0002 and 0003)
Please ignore previous email attachments.

I've only applied the v24, 0002, 0003.
seems in doc/src/sgml/ref/create_table.sgml
lack the explanation of `temporal_interval`

since foreign key ON {UPDATE | DELETE} {CASCADE,SET NULL,SET DEFAULT}
not yet supported,
v24-0003 create_table.sgml should reflect that.

+ /*
+ * For FKs with PERIOD we need an operator and aggregate function
+ * to check whether the referencing row's range is contained
+ * by the aggregated ranges of the referenced row(s).
+ * For rangetypes this is fk.periodatt <@ range_agg(pk.periodatt).
+ * FKs will look these up at "runtime", but we should make sure
+ * the lookup works here.
+ */
+ if (is_temporal)
+ FindFKPeriodOpersAndProcs(opclasses[numpks - 1], ,
);

within the function ATAddForeignKeyConstraint, you called
FindFKPeriodOpersAndProcs,
but never used the computed outputs: periodoperoid, periodprocoid,
opclasses.
We validate these(periodoperoid, periodprocoid) at
lookupTRIOperAndProc, FindFKPeriodOpersAndProcs.
I'm not sure whether FindFKPeriodOpersAndProcs in
ATAddForeignKeyConstraint is necessary.

+ * Check if all key values in OLD and NEW are "equivalent":
+ * For normal FKs we check for equality.
+ * For temporal FKs we check that the PK side is a superset of its old
value,
+ * or the FK side is a subset.
"or the FK side is a subset."  is misleading, should it be something
like "or the FK side is a subset of X"?

+ if (indexStruct->indisexclusion) return i - 1;
+ else return i;

I believe our style should be (with proper indent)
if (indexStruct->indisexclusion)
return i - 1;
else
return i;

in transformFkeyCheckAttrs
+ if (found && is_temporal)
+ {
+ found = false;
+ for (j = 0; j < numattrs + 1; j++)
+ {
+ if (periodattnum == indexStruct->indkey.values[j])
+ {
+ opclasses[numattrs] = indclass->values[j];
+ found = true;
+ break;
+ }
+ }
+ }

can be simplified:
{
found = false;
if (periodattnum == indexStruct->indkey.values[numattrs])
{
opclasses[numattrs] = indclass->values[numattrs];
found = true;
}
}

Also wondering, at the end of the function transformFkeyCheckAttrs `if
(!found)` part:
do we need another error message handle is_temporal is true?


@@ -212,8 +213,11 @@ typedef struct NewConstraint
  ConstrType contype; /* CHECK or FOREIGN */
  Oid refrelid; /* PK rel, if FOREIGN */
  Oid refindid; /* OID of PK's index, if FOREIGN */
+ bool conwithperiod; /* Whether the new FOREIGN KEY uses PERIOD */
  Oid conid; /* OID of pg_constraint entry, if FOREIGN */
  Node   *qual; /* Check expr or CONSTR_FOREIGN Constraint */
+ Oid   *operoids; /* oper oids for FOREIGN KEY with PERIOD */
+ Oid   *procoids; /* proc oids for FOREIGN KEY with PERIOD */
  ExprState  *qualstate; /* Execution state for CHECK expr */
 } NewConstraint;
primary key can only one WITHOUT OVERLAPS,
so *operoids and *procoids
can be replaced with just
`operoids, procoids`.
Also these two elements in struct NewConstraint not used in v24, 0002, 0003.
From 8ba03aa6a442d57bd0f2117e32e703fb211b68fd Mon Sep 17 00:00:00 2001
From: jian he 
Date: Fri, 2 Feb 2024 10:31:16 +0800
Subject: [PATCH v1 1/1] refactor temporal FOREIGN KEYs test

make related tests closer, remove unnecessary primary key constraint
so it improve test's readability
---
 .../regress/expected/without_overlaps.out | 76 ++-
 src/test/regress/sql/without_overlaps.sql | 76 ++-
 2 files changed, 82 insertions(+), 70 deletions(-)

diff --git a/src/test/regress/expected/without_overlaps.out b/src/test/regress/expected/without_overlaps.out
index c633738c..2d0f5e21 100644
--- a/src/test/regress/expected/without_overlaps.out
+++ b/src/test/regress/expected/without_overlaps.out
@@ -421,53 +421,58 @@ DROP TABLE temporal3;
 --
 -- test FOREIGN KEY, range references range
 --
+--test table setup.
+DROP TABLE IF EXISTS temporal_rng;
+CREATE TABLE temporal_rng (id int4range, valid_at tsrange);
+ALTER TABLE temporal_rng
+	ADD CONSTRAINT temporal_rng_pk
+	PRIMARY KEY (id, valid_at WITHOUT OVERLAPS);
 -- Can't create a FK with a mismatched range type
 CREATE TABLE temporal_fk_rng2rng (
 	id int4range,
 	valid_at int4range,
 	parent_id int4range,
-	CONSTRAINT temporal_fk_rng2rng_pk2 PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
 	CONSTRAINT temporal_fk_rng2rng_fk2 FOREIGN KEY (parent_id, PERIOD valid_at)
 		REFERENCES temporal_rng (id, PERIOD valid_at)
 );
 ERROR:  foreign key constraint "temporal_fk_rng2rng_fk2" cannot be implemented
 DETAIL:  Key 

Re: SQL:2011 application time

[jian he]

I fixed your tests, some of your tests can be simplified, (mainly
primary key constraint is unnecessary for the failed tests)
also your foreign key patch test table, temporal_rng is created at
line 141, and we use it at around line 320.
it's hard to get the definition of temporal_rng.  I drop the table
and recreate it.
So people can view the patch with tests more easily.


+ 
+  In a temporal foreign key, the delete/update will use
+  FOR PORTION OF semantics to constrain the
+  effect to the bounds being deleted/updated in the referenced row.
+ 

in v24-0003-Add-temporal-FOREIGN-KEYs.patch
 FOR PORTION OF not yet implemented, so we should
not mention it.

+ 
+  If the last column is marked with PERIOD,
+  it must be a period or range column, and the referenced table
+  must have a temporal primary key.
can we change "it must be a period or range column" to "it must be a
range column", maybe we can add it on another patch.


v1-0001-refactor-temporal-FOREIGN-KEYs-test.based_on_v24
Description: Binary data

Re: SQL:2011 application time

[Peter Eisentraut]

On 24.01.24 23:06, Paul Jungwirth wrote:

On 1/24/24 08:32, Peter Eisentraut wrote:
 > On 18.01.24 04:59, Paul Jungwirth wrote:
 >> Here are new patches consolidating feedback from several emails.
 >
 > I have committed 0001 and 0002 (the primary key support).

Thanks Peter! I noticed the comment on gist_stratnum_btree was 
out-of-date, so here is a tiny patch correcting it.


committed that

Re: SQL:2011 application time

[Peter Eisentraut]

On 18.01.24 04:59, Paul Jungwirth wrote:

Here are new patches consolidating feedback from several emails.


I have committed 0001 and 0002 (the primary key support).

The only significant tweak I did was the error messages in 
GetOperatorFromWellKnownStrategy(), to make the messages translatable 
better and share wording with other messages.  These messages are 
difficult to reach, so we'll probably have to wait for someone to 
actually encounter them to see if they are useful.


I would like to work on 0003 and 0004 (the foreign key support) during 
February/March.  The patches beyond that are probably too optimistic for 
PG17.  I recommend you focus getting 0003/0004 in good shape soon.

Re: SQL:2011 application time

[Peter Smith]

2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
there were CFbot test failures last time it was run [2]. Please have a
look and post an updated version if necessary.

==
[1] https://commitfest.postgresql.org/46/4308/
[2] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4308

Kind Regards,
Peter Smith.

Re: SQL:2011 application time

[jian he]

On Thu, Jan 11, 2024 at 10:44 PM Peter Eisentraut  wrote:
>
> On 31.12.23 09:51, Paul Jungwirth wrote:
> > On Wed, Dec 6, 2023 at 12:59 AM Peter Eisentraut 
> > wrote:
> >  >
> >  > On 02.12.23 19:41, Paul Jungwirth wrote:
> >  > > So what do you think of this idea instead?:
> >  > >
> >  > > We could add a new (optional) support function to GiST that translates
> >  > > "well-known" strategy numbers into the opclass's own strategy numbers.
> >  >
> >  > I had some conversations about this behind the scenes.  I think this
> >  > idea makes sense.
> >
> > Here is a patch series with the GiST stratnum support function added. I
> > put this into a separate patch (before all the temporal ones), so it's
> > easier to review. Then in the PK patch (now #2) we call that function to
> > figure out the = and && operators. I think this is a big improvement.
>
> I like this solution.
>
> Here is some more detailed review of the first two patches.  (I reviewed
> v20; I see you have also posted v21, but they don't appear very
> different for this purpose.)
>
> v20-0001-Add-stratnum-GiST-support-function.patch
>
> * contrib/btree_gist/Makefile
>
> Needs corresponding meson.build updates.

fixed

>
> * contrib/btree_gist/btree_gist--1.7--1.8.sql
>
> Should gist_stratnum_btree() live in contrib/btree_gist/ or in core?
> Are there other extensions that use the btree strategy numbers for
> gist?
>
> +ALTER OPERATOR FAMILY gist_vbit_ops USING gist ADD
> +   FUNCTION  12 (varbit, varbit) gist_stratnum_btree (int2) ;
>
> Is there a reason for the extra space after FUNCTION here (repeated
> throughout the file)?
>

fixed.

> +-- added in 1.4:
>
> What is the purpose of these "added in" comments?
>
>
> v20-0002-Add-temporal-PRIMARY-KEY-and-UNIQUE-constraints.patch
>
> * contrib/btree_gist/Makefile
>
> Also update meson.build.

fixed.

> * contrib/btree_gist/sql/without_overlaps.sql
>
> Maybe also insert a few values, to verify that the constraint actually
> does something?
>

I added an ok and failed INSERT.

> * doc/src/sgml/ref/create_table.sgml
>
> Is "must have a range type" still true?  With the changes to the
> strategy number mapping, any type with a supported operator class
> should work?
>
> * src/backend/utils/adt/ruleutils.c
>
> Is it actually useful to add an argument to
> decompile_column_index_array()?  Wouldn't it be easier to just print
> the " WITHOUT OVERLAPS" in the caller after returning from it?

fixed. i just print it right after decompile_column_index_array.

> * src/include/access/gist_private.h
>
> The added function gistTranslateStratnum() isn't really "private" to
> gist.  So access/gist.h would be a better place for it.
>
> Also, most other functions there appear to be named "GistSomething",
> so a more consistent name might be GistTranslateStratnum.
>
> * src/include/access/stratnum.h
>
> The added StrategyIsValid() doesn't seem that useful?  Plenty of
> existing code just compares against InvalidStrategy, and there is only
> one caller for the new function.  I suggest to do without it.
>

If more StrategyNumber are used in the future, will StrategyIsValid()
make sense?

> * src/include/commands/defrem.h
>
> We are using two terms here, well-known strategy number and canonical
> strategy number, to mean the same thing (I think?).  Let's try to
> stick with one.  Or explain the relationship?
>

In my words:
for range type, well-known strategy number and canonical strategy
number are the same thing.
For types Gist does not natively support equality, like int4,
GetOperatorFromCanonicalStrategy will pass RTEqualStrategyNumber from
ComputeIndexAttrs
and return BTEqualStrategyNumber.

> If these points are addressed, and maybe with another round of checking
> that all corner cases are covered, I think these patches (0001 and 0002)
> are close to ready.
>

the following are my review:

+ /* exclusionOpNames can be non-NIL if we are creating a partition */
+ if (iswithoutoverlaps && exclusionOpNames == NIL)
+ {
+ indexInfo->ii_ExclusionOps = palloc_array(Oid, nkeycols);
+ indexInfo->ii_ExclusionProcs = palloc_array(Oid, nkeycols);
+ indexInfo->ii_ExclusionStrats = palloc_array(uint16, nkeycols);
+ }
I am not sure the above comment is related to the code

+/*
+ * Returns the btree number for equals, otherwise invalid.
+ *
+ * This is for GiST opclasses in btree_gist (and maybe elsewhere)
+ * that use the BT*StrategyNumber constants.
+ */
+Datum
+gist_stratnum_btree(PG_FUNCTION_ARGS)
+{
+ StrategyNumber strat = PG_GETARG_UINT16(0);
+
+ switch (strat)
+ {
+ case RTEqualStrategyNumber:
+ PG_RETURN_UINT16(BTEqualStrategyNumber);
+ case RTLessStrategyNumber:
+ PG_RETURN_UINT16(BTLessStrategyNumber);
+ case RTLessEqualStrategyNumber:
+ PG_RETURN_UINT16(BTLessEqualStrategyNumber);
+ case RTGreaterStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterStrategyNumber);
+ case RTGreaterEqualStrategyNumber:
+ PG_RETURN_UINT16(BTGreaterEqualStrategyNumber);
+ default:
+ PG_RETURN_UINT16(InvalidStrategy);
+ }
the above comment seems 

Re: SQL:2011 application time

[Peter Eisentraut]

On 31.12.23 09:51, Paul Jungwirth wrote:
On Wed, Dec 6, 2023 at 12:59 AM Peter Eisentraut  
wrote:

 >
 > On 02.12.23 19:41, Paul Jungwirth wrote:
 > > So what do you think of this idea instead?:
 > >
 > > We could add a new (optional) support function to GiST that translates
 > > "well-known" strategy numbers into the opclass's own strategy numbers.
 >
 > I had some conversations about this behind the scenes.  I think this
 > idea makes sense.

Here is a patch series with the GiST stratnum support function added. I 
put this into a separate patch (before all the temporal ones), so it's 
easier to review. Then in the PK patch (now #2) we call that function to 
figure out the = and && operators. I think this is a big improvement.


I like this solution.

Here is some more detailed review of the first two patches.  (I reviewed 
v20; I see you have also posted v21, but they don't appear very 
different for this purpose.)


v20-0001-Add-stratnum-GiST-support-function.patch

* contrib/btree_gist/Makefile

Needs corresponding meson.build updates.

* contrib/btree_gist/btree_gist--1.7--1.8.sql

Should gist_stratnum_btree() live in contrib/btree_gist/ or in core?
Are there other extensions that use the btree strategy numbers for
gist?

+ALTER OPERATOR FAMILY gist_vbit_ops USING gist ADD
+   FUNCTION  12 (varbit, varbit) gist_stratnum_btree (int2) ;

Is there a reason for the extra space after FUNCTION here (repeated
throughout the file)?

+-- added in 1.4:

What is the purpose of these "added in" comments?


v20-0002-Add-temporal-PRIMARY-KEY-and-UNIQUE-constraints.patch

* contrib/btree_gist/Makefile

Also update meson.build.

* contrib/btree_gist/sql/without_overlaps.sql

Maybe also insert a few values, to verify that the constraint actually
does something?

* doc/src/sgml/ref/create_table.sgml

Is "must have a range type" still true?  With the changes to the
strategy number mapping, any type with a supported operator class
should work?

* src/backend/utils/adt/ruleutils.c

Is it actually useful to add an argument to
decompile_column_index_array()?  Wouldn't it be easier to just print
the " WITHOUT OVERLAPS" in the caller after returning from it?

* src/include/access/gist_private.h

The added function gistTranslateStratnum() isn't really "private" to
gist.  So access/gist.h would be a better place for it.

Also, most other functions there appear to be named "GistSomething",
so a more consistent name might be GistTranslateStratnum.

* src/include/access/stratnum.h

The added StrategyIsValid() doesn't seem that useful?  Plenty of
existing code just compares against InvalidStrategy, and there is only
one caller for the new function.  I suggest to do without it.

* src/include/commands/defrem.h

We are using two terms here, well-known strategy number and canonical
strategy number, to mean the same thing (I think?).  Let's try to
stick with one.  Or explain the relationship?


If these points are addressed, and maybe with another round of checking 
that all corner cases are covered, I think these patches (0001 and 0002) 
are close to ready.

Re: SQL:2011 application time

[vignesh C]

On Sat, 6 Jan 2024 at 05:50, Paul Jungwirth  
wrote:
>
> Getting caught up on reviews from November and December:
>
> On 11/19/23 22:57, jian he wrote:
>  >
>  > I believe the following part should fail. Similar tests on
>  > src/test/regress/sql/generated.sql. line begin 347.
>  >
>  > drop table if exists gtest23a,gtest23x cascade;
>  > CREATE TABLE gtest23a (x int4range, y int4range,
>  > CONSTRAINT gtest23a_pk PRIMARY KEY (x, y WITHOUT OVERLAPS));
>  > CREATE TABLE gtest23x (a int4range, b int4range GENERATED ALWAYS AS
>  > ('empty') STORED,
>  > FOREIGN KEY (a, PERIOD b ) REFERENCES gtest23a(x, PERIOD y) ON UPDATE
>  > CASCADE);  -- should be error?
>
> Okay, I've added a restriction for temporal FKs too. But note this will
> change once the PERIODs patch (the last one here) is finished. When the
> generated column is for a PERIOD, there will be logic to "reroute" the
> updates to the constituent start/end columns instead.
>
>  > begin;
>  > drop table if exists fk, pk cascade;
>  > CREATE TABLE pk (id int4range, valid_at int4range,
>  > CONSTRAINT pk_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
>  > );
>  > CREATE TABLE fk (
>  > id int4range,valid_at tsrange, parent_id int4range,
>  > CONSTRAINT fk FOREIGN KEY (parent_id, valid_at)
>  >  REFERENCES pk
>  > );
>  > rollback;
>  > --
>  > the above query will return an error: number of referencing and
>  > referenced columns for foreign key disagree.
>  > but if you look at it closely, primary key and foreign key columns both 
> are two!
>  > The error should be saying valid_at should be specified with "PERIOD".
>
> Ah okay, thanks for the clarification! This is tricky because the user
> left out the PERIOD on the fk side, and left out the entire pk side, so
> those columns are just implicit. So there is no PERIOD anywhere.
> But I agree that if the pk has WITHOUT OVERLAPS, we should expect a
> corresponding PERIOD modifier on the fk side and explain that that's
> what's missing. The attached patches include that.
>
>  > I found out other issues in v18.
>  > I first do `git apply` then  `git diff --check`, there is a white
>  > space error in v18-0005.
>
> Fixed, thanks!
>
>  > You also need to change update.sgml and delete.sgml Outputs 
> part.
>  > Since at most, it can return 'UPDATE 3' or 'DELETE 3'.
>
> This doesn't sound correct to me. An UPDATE or DELETE can target many
> rows. Also I don't think the inserted "leftovers" should be included in
> these counts. They represent the rows updated/deleted.
>
>  > --the following query should work?
>  > drop table pk;
>  > CREATE table pk(a numrange PRIMARY key,b text);
>  > insert into pk values('[1,10]');
>  > create or replace function demo1() returns void as $$
>  > declare lb numeric default 1; up numeric default 3;
>  > begin
>  >  update pk for portion of a from lb to up set b = 'lb_to_up';
>  >  return;
>  > end
>  > $$ language plpgsql;
>  > select * from demo1();
>
> Hmm this is a tough one. It is correct that the `FROM __ TO __` values cannot 
> be column references.
> They are computed up front, not per row. One reason is they are used to 
> search the table. In fact
> the standard basically allows nothing but literal strings here. See section 
> 14.14, page 971 then
> look up  on page 348 and  on page 
> 308. The most
> flexibility you get is you can add/subtract an interval to the datetime 
> literal. We are already well
> past that by allowing expressions, (certain) functions, parameters, etc.
>
> OTOH in your plpgsql example they are not really columns. They just get 
> represented as ColumnRefs
> and then passed to transformColumnRef. I'm surprised plpgsql does it that 
> way. As a workaround you
> could use `EXECUTE format(...)`, but I'd love to make that work as you show 
> instead. I'll keep
> working on this one but it's not done yet. Perhaps I can move the restriction 
> into
> analysis/planning. If anyone has any advice it'd be welcome.
>
> On 12/6/23 05:22, jian he wrote:
>  > this TODO:
>  >   * TODO: It sounds like FOR PORTION OF might need to do something here 
> too?
>  > based on comments on ExprContext. I refactor a bit, and solved this TODO.
>
> The patch looks wrong to me. We need to range targeted by `FROM __
> TO __` to live for the whole statement, not just one tuple (see just
> above). That's why it gets computed in the Init function node.
>
> I don't think that TODO is needed anymore at all. Older versions of the
> patch had more expressions besides this one, and I think it was those I
> was concerned about. So I've removed the comment here.
>
>  > tring to the following TODO:
>  > // TODO: Need to save context->mtstate->mt_transition_capture? (See
>  > comment on ExecInsert)
>  >
>  > but failed.
>  > I also attached the trial, and also added the related test.
>  >
>  > You can also use the test to check portion update with insert trigger
>  > with "referencing old table as old_table new table as new_table"
>  > situation.
>
> Thank you for the test 

Re: SQL:2011 application time

[jian he]

On Tue, Jan 9, 2024 at 2:54 AM Paul Jungwirth
 wrote:
>
> On 1/8/24 06:54, jian he wrote:
>  > On Fri, Jan 5, 2024 at 1:06 PM jian he  wrote:
>  >
>  > range_intersect returns the intersection of two ranges.
>  > I think here we are doing the opposite.
>  > names the main SQL function "range_not_intersect" and the internal
>  > function as "range_not_intersect_internal" should be fine.
>  > so people don't need to understand the meaning of "portion".
>
> Thank you for helping me figure out a name here! I realize that can be a 
> bike-sheddy kind of
> discussion, so let me share some of my principles.
>
> Range and multirange are highly mathematically "pure", and that's something I 
> value in them. It
> makes them more general-purpose, less encumbered by edge cases, easier to 
> combine, and easier to
> reason about. Preserving that close connection to math is a big goal.
>
> What I've called `without_portion` is (like) a closed form of minus (hence 
> `@-` for the operator).
> Minus isn't closed under everything (e.g. ranges), so `without_portion` adds 
> arrays---much as to
> close subtraction we add negative numbers and to close division we add 
> rationals). We get the same
> effect from multiranges, but that only buys us range support. It would be 
> awesome to support
> arbitrary types: ranges, multiranges, mdranges, boxes, polygons, inets, etc., 
> so I think an array is
> the way to go here. And then each array element is a "leftover". What do we 
> call a closed form of
> minus that returns arrays?
>
> Of course "minus" is already taken (and you wouldn't expect it to return 
> arrays anyway), which is
> why I'm thinking about names like "without" or "except". Or maybe 
> "multi-minus". I still think
> "without portion" is the closest to capturing everything above (and avoids 
> ambiguity with other SQL
> operations). And the "portion" ties the operator to `FOR PORTION OF`, which 
> is its purpose. But I
> wouldn't be surprised if there were something better.
>

Thanks for the deep explanation. I think the name
range_without_portion is better than my range_not_intersect.
I learned a lot.
I also googled " bike-sheddy". haha.

src5=# select range_without_portion(numrange(1.0,3.0,'[]'),
numrange(1.5,2.0,'(]'));
   range_without_portion
---
 {"[1.0,1.5]","(2.0,3.0]"}
(1 row)

src5=# \gdesc
Column |   Type
---+---
 range_without_portion | numeric[]
(1 row)

src5=# \df range_without_portion
 List of functions
   Schema   | Name  | Result data type | Argument data
types | Type
+---+--+-+--
 pg_catalog | range_without_portion | anyarray | anyrange,
anyrange  | func
(1 row)

so apparently, you cannot from (anyrange, anyrange) get anyarray the
element type is anyrange.
I cannot find the documented explanation in
https://www.postgresql.org/docs/current/extend-type-system.html#EXTEND-TYPES-POLYMORPHIC

anyrange is POLYMORPHIC, anyarray is POLYMORPHIC,
but I suppose, getting an anyarray the element type is anyrange would be hard.

Re: SQL:2011 application time

[Paul Jungwirth]

On 1/8/24 06:54, jian he wrote:
> On Fri, Jan 5, 2024 at 1:06 PM jian he  wrote:
>
> range_intersect returns the intersection of two ranges.
> I think here we are doing the opposite.
> names the main SQL function "range_not_intersect" and the internal
> function as "range_not_intersect_internal" should be fine.
> so people don't need to understand the meaning of "portion".

Thank you for helping me figure out a name here! I realize that can be a bike-sheddy kind of 
discussion, so let me share some of my principles.


Range and multirange are highly mathematically "pure", and that's something I value in them. It 
makes them more general-purpose, less encumbered by edge cases, easier to combine, and easier to 
reason about. Preserving that close connection to math is a big goal.


What I've called `without_portion` is (like) a closed form of minus (hence `@-` for the operator). 
Minus isn't closed under everything (e.g. ranges), so `without_portion` adds arrays---much as to 
close subtraction we add negative numbers and to close division we add rationals). We get the same 
effect from multiranges, but that only buys us range support. It would be awesome to support 
arbitrary types: ranges, multiranges, mdranges, boxes, polygons, inets, etc., so I think an array is 
the way to go here. And then each array element is a "leftover". What do we call a closed form of 
minus that returns arrays?


Using "not" suggests a function that returns true/false, but `@-` returns an array of things. So 
instead of "not" let's consider "complement". I think that's what you're expressing re intersection.


But `@-` is not the same as the complement of intersection. For one thing, `@-` is not commutative. 
`old_range @- target_portion` is not the same as `target_portion @- old_range`. But 
`complement(old_range * target_portion)` *is* the same as `complement(target_portion * old_range)`. 
Or from another angle: it's true that `old_range @- target_portion = old_range @- (old_range * 
target_portion)`, but the intersection isn't "doing" anything here. It's true that intersection and 
minus both "reduce" what you put in, but minus is more accurate.


So I think we want a name that captures that idea of "minus". Both "not" and "intersection" are 
misleading IMO.


Of course "minus" is already taken (and you wouldn't expect it to return arrays anyway), which is 
why I'm thinking about names like "without" or "except". Or maybe "multi-minus". I still think 
"without portion" is the closest to capturing everything above (and avoids ambiguity with other SQL 
operations). And the "portion" ties the operator to `FOR PORTION OF`, which is its purpose. But I 
wouldn't be surprised if there were something better.


Yours,

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

Re: SQL:2011 application time

[jian he]

On Fri, Jan 5, 2024 at 1:06 PM jian he  wrote:
>
> On Tue, Jan 2, 2024 at 9:59 AM Paul Jungwirth
>  wrote:
> >
> > On 12/31/23 00:51, Paul Jungwirth wrote:
> > > That's it for now.
> >
> > Here is another update. I fixed FOR PORTION OF on partitioned tables, in 
> > particular when the attnums
> > are different from the root partition.
> >
> > Rebased to cea89c93a1.
> >
>
> Hi.
>
> +/*
> + * range_without_portion_internal - Sets outputs and outputn to the ranges
> + * remaining and their count (respectively) after subtracting r2 from r1.
> + * The array should never contain empty ranges.
> + * The outputs will be ordered. We expect that outputs is an array of
> + * RangeType pointers, already allocated with two slots.
> + */
> +void
> +range_without_portion_internal(TypeCacheEntry *typcache, RangeType *r1,
> +   RangeType *r2, RangeType **outputs, int *outputn)
> I am confused with the name "range_without_portion", I think
> "range_not_overlap" would be better.
>

range_intersect returns the intersection of two ranges.
I think here we are doing the opposite.
names the main SQL function "range_not_intersect" and the internal
function as "range_not_intersect_internal" should be fine.
so people don't need to understand the meaning of "portion".

Re: SQL:2011 application time

[jian he]

On Tue, Jan 2, 2024 at 9:59 AM Paul Jungwirth
 wrote:
>
> On 12/31/23 00:51, Paul Jungwirth wrote:
> > That's it for now.
>
> Here is another update. I fixed FOR PORTION OF on partitioned tables, in 
> particular when the attnums
> are different from the root partition.
>
> Rebased to cea89c93a1.
>

Hi.

+/*
+ * range_without_portion_internal - Sets outputs and outputn to the ranges
+ * remaining and their count (respectively) after subtracting r2 from r1.
+ * The array should never contain empty ranges.
+ * The outputs will be ordered. We expect that outputs is an array of
+ * RangeType pointers, already allocated with two slots.
+ */
+void
+range_without_portion_internal(TypeCacheEntry *typcache, RangeType *r1,
+   RangeType *r2, RangeType **outputs, int *outputn)
+{
+ int cmp_l1l2,
+ cmp_l1u2,
+ cmp_u1l2,
+ cmp_u1u2;
+ RangeBound lower1,
+ lower2;
+ RangeBound upper1,
+ upper2;
+ bool empty1,
+ empty2;
+
+ range_deserialize(typcache, r1, , , );
+ range_deserialize(typcache, r2, , , );
+
+ if (empty1)
+ {
+ /* if r1 is empty then r1 - r2 is empty, so return zero results */
+ *outputn = 0;
+ return;
+ }
+ else if (empty2)
+ {
+ /* r2 is empty so the result is just r1 (which we know is not empty) */
+ outputs[0] = r1;
+ *outputn = 1;
+ return;
+ }
+
+ /*
+ * Use the same logic as range_minus_internal,
+ * but support the split case
+ */
+ cmp_l1l2 = range_cmp_bounds(typcache, , );
+ cmp_l1u2 = range_cmp_bounds(typcache, , );
+ cmp_u1l2 = range_cmp_bounds(typcache, , );
+ cmp_u1u2 = range_cmp_bounds(typcache, , );
+
+ if (cmp_l1l2 < 0 && cmp_u1u2 > 0)
+ {
+ lower2.inclusive = !lower2.inclusive;
+ lower2.lower = false; /* it will become the upper bound */
+ outputs[0] = make_range(typcache, , , false, NULL);
+
+ upper2.inclusive = !upper2.inclusive;
+ upper2.lower = true; /* it will become the lower bound */
+ outputs[1] = make_range(typcache, , , false, NULL);
+
+ *outputn = 2;
+ }
+ else if (cmp_l1u2 > 0 || cmp_u1l2 < 0)
+ {
+ outputs[0] = r1;
+ *outputn = 1;
+ }
+ else if (cmp_l1l2 >= 0 && cmp_u1u2 <= 0)
+ {
+ *outputn = 0;
+ }
+ else if (cmp_l1l2 <= 0 && cmp_u1l2 >= 0 && cmp_u1u2 <= 0)
+ {
+ lower2.inclusive = !lower2.inclusive;
+ lower2.lower = false; /* it will become the upper bound */
+ outputs[0] = make_range(typcache, , , false, NULL);
+ *outputn = 1;
+ }
+ else if (cmp_l1l2 >= 0 && cmp_u1u2 >= 0 && cmp_l1u2 <= 0)
+ {
+ upper2.inclusive = !upper2.inclusive;
+ upper2.lower = true; /* it will become the lower bound */
+ outputs[0] = make_range(typcache, , , false, NULL);
+ *outputn = 1;
+ }
+ else
+ {
+ elog(ERROR, "unexpected case in range_without_portion");
+ }
+}

I am confused.
say condition: " (cmp_l1l2 <= 0 && cmp_u1l2 >= 0 && cmp_u1u2 <= 0)"
the following code will only run PartA, never run PartB?

`
else if (cmp_l1l2 >= 0 && cmp_u1u2 <= 0)
PartA
else if (cmp_l1l2 <= 0 && cmp_u1l2 >= 0 && cmp_u1u2 <= 0)
PartB
`

minimum example:
#include
#include
#include
#include
int
main(void)
{
int cmp_l1l2;
int cmp_u1u2;
int cmp_u1l2;
int cmp_l1u2;
cmp_l1u2 = -1;
cmp_l1l2 = 0;
cmp_u1u2 = 0;
cmp_u1l2 = 0;
assert(cmp_u1l2 == 0);
if (cmp_l1u2 > 0 || cmp_u1l2 < 0)
printf("calling partA\n");
else if (cmp_l1l2 >= 0 && cmp_u1u2 <= 0)
printf("calling partB\n");
else if (cmp_l1l2 <= 0 && cmp_u1l2 >= 0 && cmp_u1u2 <= 0)
printf("calling partC\n");
}

I am confused with the name "range_without_portion", I think
"range_not_overlap" would be better.

select numrange(1.1, 2.2) @- numrange(2.0, 3.0);
the result is not the same as
select numrange(2.0, 3.0) @- numrange(1.1, 2.2);

So your categorize oprkind as 'b' for operator "@-" is wrong?
select oprname,oprkind,oprcanhash,oprcanmerge,oprleft,oprright,oprresult,oprcode
from pg_operator
where oprname = '@-';

aslo
select count(*), oprkind from pg_operator group by oprkind;
there are only 5% are prefix operators.
maybe we should design it as:
1. if both inputs are empty range, the result array is empty.
2. if both inputs are non-empty and never overlaps, put both of them
to the result array.
3. if one input is empty another one is not, then put the non-empty
one into the result array.

after applying the patch: now the catalog data seems not correct to me.
SELECT  a1.amopfamily
,a1.amoplefttype::regtype
,a1.amoprighttype
,a1.amopstrategy
,amoppurpose
,amopsortfamily
,amopopr
,op.oprname
,am.amname
FROMpg_amop as a1 join pg_operator op on op.oid = a1.amopopr
joinpg_am   am on am.oid = a1.amopmethod
where   amoppurpose = 'p';
output:
 amopfamily | amoplefttype  | amoprighttype | amopstrategy |
amoppurpose | amopsortfamily | amopopr | oprname | amname
+---+---+--+-++-+-+
   2593 | box   |   603 |   31 | p
  |  0 | 803 | #   | gist
   3919 | anyrange  |  3831 |   

Re: SQL:2011 application time

[jian he]

hi. some small issues

diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index e3ccf6c7f7..6781e55020 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1560,7 +1560,7 @@ ProcessUtilitySlow(ParseState *pstate,
  true, /* check_rights */
  true, /* check_not_in_use */
  false, /* skip_build */
- false); /* quiet */
+ false); /* quiet */

Is the above part unnecessary?

diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 199eae525d..d04c75b398 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -78,6 +78,7 @@ OBJS = \
  oracle_compat.o \
  orderedsetaggs.o \
  partitionfuncs.o \
+ period.o \
  pg_locale.o \
  pg_lsn.o \
  pg_upgrade_support.o \
diff --git a/src/backend/utils/adt/period.c b/src/backend/utils/adt/period.c
new file mode 100644
index 00..0ed4304e16
--- /dev/null
+++ b/src/backend/utils/adt/period.c
@@ -0,0 +1,56 @@
+/*-
+ *
+ * period.c
+ *   Functions to support periods.
+ *
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *   src/backend/utils/adt/period.c
+ *
+ *-
+ */
+#include "postgres.h"
+
+#include "executor/tuptable.h"
+#include "fmgr.h"
+#include "nodes/primnodes.h"
+#include "utils/fmgrprotos.h"
+#include "utils/period.h"
+#include "utils/rangetypes.h"
+
+Datum period_to_range(TupleTableSlot *slot, int startattno, int
endattno, Oid rangetype)
+{
+ Datum startvalue;
+ Datum endvalue;
+ Datum result;
+ bool startisnull;
+ bool endisnull;
+ LOCAL_FCINFO(fcinfo, 2);
+ FmgrInfo flinfo;
+ FuncExpr   *f;
+
+ InitFunctionCallInfoData(*fcinfo, , 2, InvalidOid, NULL, NULL);
+ f = makeNode(FuncExpr);
+ f->funcresulttype = rangetype;
+ flinfo.fn_expr = (Node *) f;
+ flinfo.fn_extra = NULL;
+
+ /* compute oldvalue */
+ startvalue = slot_getattr(slot, startattno, );
+ endvalue = slot_getattr(slot, endattno, );
+
+ fcinfo->args[0].value = startvalue;
+ fcinfo->args[0].isnull = startisnull;
+ fcinfo->args[1].value = endvalue;
+ fcinfo->args[1].isnull = endisnull;
+
+ result = range_constructor2(fcinfo);
+ if (fcinfo->isnull)
+ elog(ERROR, "function %u returned NULL", flinfo.fn_oid);
+
+ return result;
+}

I am confused. so now I only apply v19, 0001 to 0003.
period_to_range function never used. maybe we can move this part to
0005-Add PERIODs.patch?
Also you add change in Makefile in 0003, meson.build change in 0005,
better put it on in 0005?

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5b110ca7fe..d54d84adf6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y

+/*
+ * We need to handle this shift/reduce conflict:
+ * FOR PORTION OF valid_at FROM INTERVAL YEAR TO MONTH TO foo.
+ * This is basically the classic "dangling else" problem, and we want a
+ * similar resolution: treat the TO as part of the INTERVAL, not as part of
+ * the FROM ... TO  Users can add parentheses if that's a problem.
+ * TO just needs to be higher precedence than YEAR_P etc.
+ * TODO: I need to figure out a %prec solution before this gets committed!
+ */
+%nonassoc YEAR_P MONTH_P DAY_P HOUR_P MINUTE_P
+%nonassoc TO

this part will never happen?
since "FROM INTERVAL YEAR TO MONTH TO"
means "valid_at" will be interval range data type, which does not exist now.

  ri_PerformCheck(riinfo, , qplan,
  fk_rel, pk_rel,
  oldslot, NULL,
+ targetRangeParam, targetRange,
  true, /* must detect new rows */
  SPI_OK_SELECT);

@@ -905,6 +922,7 @@ RI_FKey_cascade_del(PG_FUNCTION_ARGS)
  ri_PerformCheck(riinfo, , qplan,
  fk_rel, pk_rel,
  oldslot, NULL,
+ -1, 0,
  true, /* must detect new rows */
  SPI_OK_DELETE);

@@ -1026,6 +1044,7 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS)
  ri_PerformCheck(riinfo, , qplan,
  fk_rel, pk_rel,
  oldslot, newslot,
+ -1, 0,
  true, /* must detect new rows */
  SPI_OK_UPDATE);

@@ -1258,6 +1277,7 @@ ri_set(TriggerData *trigdata, bool is_set_null,
int tgkind)
  ri_PerformCheck(riinfo, , qplan,
  fk_rel, pk_rel,
  oldslot, NULL,
+ -1, 0,
  true, /* must detect new rows */
  SPI_OK_UPDATE);

@@ -2520,6 +2540,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
  RI_QueryKey *qkey, SPIPlanPtr qplan,
  Relation fk_rel, Relation pk_rel,
  TupleTableSlot *oldslot, TupleTableSlot *newslot,
+ int forPortionOfParam, Datum forPortionOf,
  bool detectNewRows, int expect_OK)

for all the refactor related to ri_PerformCheck, do you need (Datum) 0
instead of plain 0?

+  
+   If the table has a range column or
+   PERIOD,
+   you may supply a FOR PORTION OF clause, and
your delete will
+   only affect rows that overlap the given interval. Furthermore, if
a row's span

Re: SQL:2011 application time

[jian he]

On Sun, Dec 3, 2023 at 2:11 AM Paul Jungwirth
 wrote:
>
> v19 patch series attached, rebased to a11c9c42ea.
>

this TODO:
 * TODO: It sounds like FOR PORTION OF might need to do something here too?
based on comments on ExprContext. I refactor a bit, and solved this TODO.

tring to the following TODO:
// TODO: Need to save context->mtstate->mt_transition_capture? (See
comment on ExecInsert)

but failed.
I also attached the trial, and also added the related test.

You can also use the test to check portion update with insert trigger
with "referencing old table as old_table new table as new_table"
situation.
From 8c97db9391900d766db99834c23c5e4b60cadf01 Mon Sep 17 00:00:00 2001
From: pgaddict 
Date: Tue, 5 Dec 2023 17:59:43 +0800
Subject: [PATCH v1 1/1] set eval targetrange in per-output-tuple exprcontext

---
 src/backend/executor/nodeModifyTable.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 88eda012..fe57e592 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -3780,7 +3780,6 @@ ExecModifyTable(PlanState *pstate)
 		 * Reset per-tuple memory context used for processing on conflict and
 		 * returning clauses, to free any expression evaluation storage
 		 * allocated in the previous cycle.
-		 * TODO: It sounds like FOR PORTION OF might need to do something here too?
 		 */
 		if (pstate->ps_ExprContext)
 			ResetExprContext(pstate->ps_ExprContext);
@@ -4472,9 +4471,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
 		forPortionOf = (ForPortionOfExpr *) node->forPortionOf;
 
 		/* Eval the FOR PORTION OF target */
-		if (mtstate->ps.ps_ExprContext == NULL)
-			ExecAssignExprContext(estate, >ps);
-		econtext = mtstate->ps.ps_ExprContext;
+		econtext = GetPerTupleExprContext(estate);
 
 		exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange, estate);
 		targetRange = ExecEvalExpr(exprState, econtext, );
-- 
2.34.1

From cda2f8324ef920c807008e2763f97c6503c17a94 Mon Sep 17 00:00:00 2001
From: pgaddict 
Date: Wed, 6 Dec 2023 20:58:08 +0800
Subject: [PATCH v1 1/1] trying to save mt_transition_capture while ExecInsert

---
 src/backend/executor/nodeModifyTable.c   | 21 -
 src/test/regress/expected/for_portion_of.out | 80 
 src/test/regress/sql/for_portion_of.sql  | 72 ++
 3 files changed, 171 insertions(+), 2 deletions(-)

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index fe57e592..c9d14dab 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -167,7 +167,8 @@ static bool ExecMergeMatched(ModifyTableContext *context,
 static void ExecMergeNotMatched(ModifyTableContext *context,
 ResultRelInfo *resultRelInfo,
 bool canSetTag);
-
+static void
+ExecSetupTransitionCaptureState(ModifyTableState *mtstate, EState *estate);

 /*
  * Verify that the tuples to be produced by INSERT match the
@@ -1259,7 +1260,7 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
 	TupleTableSlot *leftoverTuple2 = fpoState->fp_Leftover2;
 	HeapTuple oldtuple = NULL;
 	bool shouldFree = false;
-
+	int		i;	/* original cmd type */
 	/*
 	 * Get the range of the old pre-UPDATE/DELETE tuple,
 	 * so we can intersect it with the FOR PORTION OF target
@@ -1312,6 +1313,7 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
 	 * TODO: Really? What if you update the partition key?
 	 */

+	i	= context->mtstate->operation;
 	if (!RangeIsEmpty(leftoverRangeType1))
 	{
 		oldtuple = ExecFetchSlotHeapTuple(oldtupleSlot, false, );
@@ -1320,6 +1322,10 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
 		set_leftover_tuple_bounds(leftoverTuple1, forPortionOf, typcache, leftoverRangeType1);
 		ExecMaterializeSlot(leftoverTuple1);

+		context->mtstate->operation = CMD_INSERT;
+		context->mtstate->mt_transition_capture = NULL;
+		ExecSetupTransitionCaptureState(context->mtstate, estate);
+
 		// TODO: Need to save context->mtstate->mt_transition_capture? (See comment on ExecInsert)
 		ExecInsert(context, resultRelInfo, leftoverTuple1, node->canSetTag, NULL, NULL);
 	}
@@ -1340,10 +1346,21 @@ ExecForPortionOfLeftovers(ModifyTableContext *context,
 		set_leftover_tuple_bounds(leftoverTuple2, forPortionOf, typcache, leftoverRangeType2);
 		ExecMaterializeSlot(leftoverTuple2);

+		context->mtstate->operation = CMD_INSERT;
+		context->mtstate->mt_transition_capture = NULL;
+		ExecSetupTransitionCaptureState(context->mtstate, estate);
+
 		// TODO: Need to save context->mtstate->mt_transition_capture? (See comment on ExecInsert)
 		ExecInsert(context, resultRelInfo, leftoverTuple2, node->canSetTag, NULL, NULL);
 	}

+	if (!RangeIsEmpty(leftoverRangeType2) || !RangeIsEmpty(leftoverRangeType1))
+	{
+		/* if any of the above branch executed then */
+		context->mtstate->operation = i;
+		

Re: SQL:2011 application time

[Peter Eisentraut]

On 02.12.23 19:41, Paul Jungwirth wrote:

So what do you think of this idea instead?:

We could add a new (optional) support function to GiST that translates 
"well-known" strategy numbers into the opclass's own strategy numbers. 
This would be support function 12. Then we can say 
translateStrategyNumber(RTEqualStrategyNumber) and look up the operator 
with the result.


There is not a performance hit, because we do this for the DDL command 
(create pk/uq/fk), then store the operator in the index/constraint.


If you don't provide this new support function, then creating the 
pk/uq/fk fails with a hint about what you can do to make it work.


This approach means we don't change the rules about GiST opclasses: you 
can still use the stranums how you like.


This function would also let me support non-range "temporal" foreign 
keys, where I'll need to build queries with && and maybe other operators.


I had some conversations about this behind the scenes.  I think this 
idea makes sense.


The other idea was that we create new strategy numbers, like 
TemporalEqualsStrategy / TemporalOverlapsStrategy.  But then you'd have 
the situation where some strategy numbers are reserved and others are 
not, so perhaps that is not so clean.  I think your idea is good.

Re: SQL:2011 application time

[Vik Fearing]

On 12/2/23 19:11, Paul Jungwirth wrote:

Thank you again for such thorough reviews!

On Thu, Nov 16, 2023 at 11:12 PM jian he  
wrote:
 > UPDATE FOR PORTION OF, may need insert privilege. We also need to 
document this.
 > Similarly, we also need to apply the above logic to DELETE FOR 
PORTION OF.


I don't think UPDATE/DELETE FOR PORTION OF is supposed to require INSERT 
permission.


Notionally the INSERTs are just to preserve what was there already, not 
to add new data.
The idea is that a temporal table is equivalent to a table with one row 
for every "instant",
i.e. one row per microsecond/second/day/whatever-time-resolution. Of 
course that would be too slow,
so we use PERIODs/ranges instead, but the behavior should be the same. 
Date's book has a good discussion of this idea.


I also checked the SQL:2011 draft standard, and there is a section 
called Access Rules in Part 2: SQL/Foundation for UPDATE and DELETE 
statements. Those sections say you need UPDATE/DELETE privileges, but 
say nothing about needing INSERT privileges. That is on page 949 and 972 
of the PDFs from the "SQL:20nn Working Draft Documents" link at [1]. If 
someone has a copy of SQL:2016 maybe something was changed, but I would 
be surprised


Nothing has changed here in SQL:2023 (or since).
--
Vik Fearing

Re: SQL:2011 application time

[Paul Jungwirth]

On Thu, Nov 23, 2023 at 1:08 AM Peter Eisentraut  wrote:
> After further thought, I think the right solution is to change
> btree_gist (and probably also btree_gin) to use the common RT* strategy
> numbers.

Okay. That will mean bumping the version of btree_gist, and you must be running that version to use 
the new temporal features, or you will get silly results. Right? Is there a way to protect users 
against that and communicate they need to upgrade the extension?


This also means temporal features may not work in custom GIST opclasses. What we're saying is they 
must have an appropriate operator for RTEqualStrategyNumber (18) and RTOverlapStrategyNumber (3). 
Equal matters for the scalar key part(s), overlap for the range part. So equal is more likely to be 
an issue, but overlap matters if we want to support non-ranges (which I'd say is worth doing).


Also if they get it wrong, we won't really have any way to report an error.

I did some research on other extensions in contrib, as well as PostGIS. Here is 
what I found:

## btree_gin:

3 is =
18 is undefined

same for all types: macaddr8, int2, int4, int8, float4, float8, oid, timestamp, timestamptz, time, 
timetz, date, interval, inet, cidr, text, varchar, char, bytea, bit, varbit, numeric, anyenum, uuid, 
name, bool, bpchar


## cube

3 is &&
18 is <=>

## intarray

3 is &&
18 is undefined

## ltree

3 is =
18 is undefined

## hstore

3 and 18 are undefined

## seg

3 is &&
18 is undefined

## postgis: geometry

3 is &&
18 is undefined

## postgis: geometry_nd

3 is &&&
18 is undefined

I thought about looking through pgxn for more, but I haven't yet. I may still 
do that.
But already it seems like there is not much consistency.

So what do you think of this idea instead?:

We could add a new (optional) support function to GiST that translates "well-known" strategy numbers 
into the opclass's own strategy numbers. This would be support function 12. Then we can say 
translateStrategyNumber(RTEqualStrategyNumber) and look up the operator with the result.


There is not a performance hit, because we do this for the DDL command (create pk/uq/fk), then store 
the operator in the index/constraint.


If you don't provide this new support function, then creating the pk/uq/fk fails with a hint about 
what you can do to make it work.


This approach means we don't change the rules about GiST opclasses: you can still use the stranums 
how you like.


This function would also let me support non-range "temporal" foreign keys, where I'll need to build 
queries with && and maybe other operators.


What do you think?

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

Re: SQL:2011 application time

[Peter Eisentraut]

On 20.11.23 08:58, Peter Eisentraut wrote:

On 17.11.23 19:39, Paul Jungwirth wrote:
But I feel the overall approach is wrong: originally I used hardcoded 
"=" and "&&" operators, and you asked me to look them up by strategy 
number instead. But that leads to trouble with core gist types vs 
btree_gist types. The core gist opclasses use RT*StrategyNumbers, but 
btree_gist creates opclasses with BT*StrategyNumbers.


Ouch.

That also provides the answer to my question #2 here: 
https://www.postgresql.org/message-id/6f010a6e-8e20-658b-dc05-dc9033a694da%40eisentraut.org


I don't have a good idea about this right now.  Could we just change 
btree_gist perhaps?  Do we need a new API for this somewhere?


After further thought, I think the right solution is to change 
btree_gist (and probably also btree_gin) to use the common RT* strategy 
numbers.  The strategy numbers are the right interface to determine the 
semantics of index AM operators.  It's just that until now, nothing 
external has needed this information from gist indexes (unlike btree, 
hash), so it has been a free-for-all.


I don't see an ALTER OPERATOR CLASS command that could be used to 
implement this.  Maybe we could get away with a direct catalog UPDATE. 
Or we need to make some DDL for this.


Alternatively, this could be the time to reconsider moving this into core.

Re: SQL:2011 application time

[Peter Eisentraut]

On 17.11.23 19:39, Paul Jungwirth wrote:
But I feel the overall approach is wrong: originally I used hardcoded 
"=" and "&&" operators, and you asked me to look them up by strategy 
number instead. But that leads to trouble with core gist types vs 
btree_gist types. The core gist opclasses use RT*StrategyNumbers, but 
btree_gist creates opclasses with BT*StrategyNumbers.


Ouch.

That also provides the answer to my question #2 here: 
https://www.postgresql.org/message-id/6f010a6e-8e20-658b-dc05-dc9033a694da%40eisentraut.org


I don't have a good idea about this right now.  Could we just change 
btree_gist perhaps?  Do we need a new API for this somewhere?

Re: SQL:2011 application time

[jian he]

On Sun, Nov 19, 2023 at 1:24 PM Paul A Jungwirth
 wrote:
>
> 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  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?
>

I believe the following part should fail. Similar tests on
src/test/regress/sql/generated.sql. line begin 347.

drop table if exists gtest23a,gtest23x cascade;
CREATE TABLE gtest23a (x int4range, y int4range,
CONSTRAINT gtest23a_pk PRIMARY KEY (x, y WITHOUT OVERLAPS));
CREATE TABLE gtest23x (a int4range, b int4range GENERATED ALWAYS AS
('empty') STORED,
FOREIGN KEY (a, PERIOD b ) REFERENCES gtest23a(x, PERIOD y) ON UPDATE
CASCADE);  -- should be error?
---

>
> > * 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."
>

begin;
drop table if exists fk, pk cascade;
CREATE TABLE pk (id int4range, valid_at int4range,
CONSTRAINT pk_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
);
CREATE TABLE fk (
id int4range,valid_at tsrange, parent_id int4range,
CONSTRAINT fk FOREIGN KEY (parent_id, valid_at)
REFERENCES pk
);
rollback;
--
the above query will return an error: number of referencing and
referenced columns for foreign key disagree.
but if you look at it closely, primary key and foreign key columns both are two!
The error should be saying valid_at should be specified with "PERIOD".

begin;
drop table if exists fk, pk cascade;
CREATE TABLE pk (id int4range, valid_at int4range,
CONSTRAINT pk_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
);
CREATE TABLE fk (
id int4range,valid_at int4range, parent_id int4range,
CONSTRAINT fk FOREIGN KEY (parent_id, period valid_at)
REFERENCES pk
);
select conname,array_length(conkey,1),array_length(confkey,1)
from pg_constraint where conname = 'fk';
rollback;

I found out other issues in v18.
I first do `git apply` then  `git diff --check`, there is a white
space error in v18-0005.

You also need to change update.sgml and delete.sgml Outputs part.
Since at most, it can return 'UPDATE 3' or 'DELETE 3'.

--the following query should work?
drop table pk;
CREATE table pk(a numrange PRIMARY key,b text);
insert into pk values('[1,10]');
create or replace function demo1() returns void as $$
declare lb numeric default 1; up numeric default 3;
begin
update pk for portion of a from lb to up set b = 'lb_to_up';
return;
end
$$ language plpgsql;
select * from demo1();

Re: SQL:2011 application time

[Paul A Jungwirth]

On Mon, Nov 6, 2023 at 11:07 PM jian he  wrote:
> + 
> +  In a temporal foreign key, the delete/update will use
> +  FOR PORTION OF semantics to constrain the
> +  effect to the bounds being deleted/updated in the referenced row.
> + 
>
> The first "para" should be  ?

Thanks, fixed (in v18)!

> There are many warnings after #define WRITE_READ_PARSE_PLAN_TREES
> see: http://cfbot.cputube.org/highlights/all.html#4308
> Does that mean oue new change in gram.y is somehow wrong?

Fixed (in read+out node funcs).

> sgml/html/sql-update.html:
> "range_or_period_name
> The range column or period to use when performing a temporal update.
> This must match the range or period used in the table's temporal
> primary key."
>
> Is the second sentence unnecessary? since no primary can still do "for
> portion of update".

You're right, this dates back to an older version of the patch. Removed.

> sgml/html/sql-update.html:
> "start_time
> The earliest time (inclusive) to change in a temporal update. This
> must be a value matching the base type of the range or period from
> range_or_period_name. It may also be the special value MINVALUE to
> indicate an update whose beginning is unbounded."
>
> probably something like the following:
> "lower_bound"
> The lower bound (inclusive) to change in an overlap update. This must
> be a value matching the base type of the range or period from
> range_or_period_name. It may also be the special value UNBOUNDED to
> indicate an update whose beginning is unbounded."
>
> Obviously the "start_time" reference also needs to change, and the
> sql-delete.html reference also needs to change.

See below re UNBOUNDED

> UPDATE for_portion_of_test FOR PORTION OF valid_at  FROM NULL TO
> "unbounded" SET name = 'NULL to NULL';
> should fail, but not. double quoted unbounded is a column reference, I assume.
>
> That's why I am confused with the function transformForPortionOfBound.
> "if (nodeTag(n) == T_ColumnRef)" part.

You're right, using a ColumnDef was probably not good here, and
treating `"UNBOUNDED"` (with quotes from the user) as a keyword is no
good. I couldn't find a way to make this work without reduce/reduce
conflicts, so I just took it out. It was syntactic sugar for `FROM/TO
NULL` and not part of the standard, so it's not too important. Also I
see that UNBOUNDED causes difficult problems already with window
functions (comments in gram.y). I hope I can find a way to make this
work eventually, but it can go for now.

> in create_table.sgml. you also need to add  WITHOUT OVERLAPS related
> info into 

You're right, fixed (though Peter's patch then changed this same spot).

Thanks,

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

Re: SQL:2011 application time

[Paul A Jungwirth]

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  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  ~{:-)
p...@illuminatedcomputing.com

Re: SQL:2011 application time

[jian he]

based on v17.

begin;
drop table if exists s1;
CREATE TABLE s1 (id numrange, misc int, misc1 text);
create role  test101 login;
grant update, select  on s1 to test101;
insert into s1 VALUES ('[1,1000]',2);
set session authorization test101;
update s1 set id = '[1,1000]';
savepoint sp1;
update s1 FOR PORTION OF id from 10 to 100 set misc1 = 'test';
table s1;
savepoint sp2;
insert into s1 VALUES ('[2,1000]',12);
rollback;

In UPDATE FOR PORTION OF from x to y, if range [x,y) overlaps with the
"source" range
then the UPDATE action would be UPDATE and INSERT.
The above UPDATE FOR PORTION OF query should fail?
UPDATE FOR PORTION OF, may need insert privilege. We also need to document this.
Similarly, we also need to apply the above logic to DELETE FOR PORTION OF.
---
+  
+   If the table has a range column
+   or PERIOD, you may supply a

should be

+ 
+  If the table has a range column or  
+  PERIOD, you may supply a

similarly the doc/src/sgml/ref/delete.sgml the link reference also broken.

  
   If the table has a range column or  
  PERIOD, you may supply a
   FOR PORTION OF clause, and your update will only
affect rows
   that overlap the given interval. Furthermore, if a row's span extends outside
   the FOR PORTION OF bounds, then it will be
truncated to fit
   within the bounds, and new rows spanning the "cut off" duration will be
   inserted to preserve the old values.
  

 "given interval", "cut off" these words,  imho, feel not so clear.
We also need a document that:
 "UPDATE FOR PORTION OF" is UPDATE and INSERT (if overlaps).
If the "UPDATE FOR PORTION OF" range overlaps then
It will invoke triggers in the following order: before row update,
before row insert, after row insert. after row update.
---
src/test/regress/sql/for_portion_of.sql
You only need to create two triggers?
since for_portion_of_trigger only raises notice to output the triggers
meta info.

CREATE TRIGGER trg_for_portion_of_before
  BEFORE INSERT OR UPDATE OR DELETE ON for_portion_of_test
  FOR EACH ROW
  EXECUTE FUNCTION for_portion_of_trigger();
CREATE TRIGGER trg_for_portion_of_after
AFTER INSERT OR UPDATE OR DELETE ON for_portion_of_test
FOR EACH ROW
EXECUTE FUNCTION for_portion_of_trigger();

Re: SQL:2011 application time

[Paul Jungwirth]

On 11/9/23 05:47, Peter Eisentraut wrote:
I went over the patch 
v17-0001-Add-temporal-PRIMARY-KEY-and-UNIQUE-constraints.patch in more 
detail


Thanks Peter!

I'm about halfway through jian he's last two emails. I'll address your 
feedback also. I wanted to reply to this without waiting though:


Overall, with these fixes, I think this patch is structurally okay.  We 
just need to make sure we have all the weird corner cases covered.


One remaining issue I know about is with table partitions whose column 
order has changed. I've got an in-progress fix for that, but I've been 
prioritizing reviewer feedback the last few months. Just want to make 
sure you know about it for now.


Thanks!

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

Re: SQL:2011 application time

[Peter Eisentraut]

On 02.11.23 21:21, Paul Jungwirth wrote:

New patches attached (rebased to 0bc726d9).


I went over the patch 
v17-0001-Add-temporal-PRIMARY-KEY-and-UNIQUE-constraints.patch in more 
detail.  Attached is a fixup patch that addresses a variety of cosmetic 
issues.


Some details:

- Renamed contemporal to conwithoutoverlaps, as previously discussed. 
Also renamed various variables and function arguments similarly.


- Rearranged text in CREATE TABLE reference page so there are no forward 
references.  (Describe WITHOUT OVERLAPS under UNIQUE and then PRIMARY 
KEy says "see above", rather than describe it under PRIMARY KEY and have 
UNIQUE say "see below.)


- Removed various bits that related to temporal foreign keys, which 
belong in a later patch.


- Reverted some apparently unrelated changes in src/backend/catalog/index.c.

- Removed the "temporal UNIQUE" constraint_type assignment in 
DefineIndex().  This is meant to be used in error messages and should 
refer to actual syntax.  I think it's fine without it this change.


- Field contemporal in NewConstraint struct is not used by this patch.

- Rearrange the grammar so that the rule with WITHOUT OVERLAPS is just a 
Boolean attribute rather than column name plus keywords.  This was kind 
of confusing earlier and led to weird error messages for invalid syntax. 
 I kept the restriction that you need at least one non-overlaps column, 
but that is now enforced in parse analysis, not in the grammar.  (But 
maybe we don't need it?)


(After your earlier explanation, I'm content to just allow one WITHOUT 
OVERLAPS column for now.)


- Some places looked at conexclop to check whether something is a 
WITHOUT OVERLAPS constraint, instead of looking at conwithoutoverlaps 
directly.


- Removed some redundant "unlike" entries in the pg_dump tests.  (This 
caused cfbot tests to fail.)


- Moved the "without_overlaps" test later in the schedule.  It should at 
least be after "constraints" so that normal constraints are tested first.



Two areas that could be improved:

1) In src/backend/commands/indexcmds.c, 
get_index_attr_temporal_operator() has this comment:


+* This seems like a hack
+* but I can't find any existing lookup function
+* that knows about pseudotypes.

This doesn't see very confident. ;-)  I don't quite understand this.  Is 
this a gap in the currently available APIs, do we need to improve 
something here, or does this need more research?


2) In src/backend/parser/parse_utilcmd.c, transformIndexConstraint(), 
there is too much duplication between the normal and the if 
(constraint->without_overlaps) case, like the whole not-null constraints 
stuff at the end.  This should be one code block with a few conditionals 
inside.  Also, the normal case deals with things like table inheritance, 
which the added parts do not.  Is this all complete?


I'm not sure the validateWithoutOverlaps() function is needed at this 
point in the code.  We just need to check that the column exists, which 
the normal code path already does, and then have the index creation code 
later check that an appropriate overlaps operator exists.  We don't even 
need to restrict this to range types.  Consider for example, it's 
possible that a type does not have a btree equality operator.  We don't 
check that here either, but let the index code later check it.



Overall, with these fixes, I think this patch is structurally okay.  We 
just need to make sure we have all the weird corner cases covered.
From 6d7af1f78505c08fb205a56bd1ba3cb951f04002 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 9 Nov 2023 14:11:34 +0100
Subject: [PATCH] fixup! Add temporal PRIMARY KEY and UNIQUE constraints

---
 doc/src/sgml/catalogs.sgml|  8 +--
 doc/src/sgml/ref/create_table.sgml| 68 ---
 src/backend/catalog/heap.c|  4 +-
 src/backend/catalog/index.c   | 14 ++--
 src/backend/catalog/pg_constraint.c   |  4 +-
 src/backend/commands/indexcmds.c  | 30 
 src/backend/commands/tablecmds.c  | 16 ++---
 src/backend/commands/trigger.c|  2 +-
 src/backend/commands/typecmds.c   |  2 +-
 src/backend/parser/gram.y | 19 +++---
 src/backend/parser/parse_utilcmd.c| 28 ++--
 src/backend/utils/adt/ruleutils.c | 27 +++-
 src/backend/utils/cache/relcache.c| 16 +++--
 src/bin/pg_dump/pg_dump.c | 33 +++--
 src/bin/pg_dump/pg_dump.h |  2 +-
 src/bin/pg_dump/t/002_pg_dump.pl  | 14 ++--
 src/bin/psql/describe.c   | 10 +--
 src/include/catalog/index.h   |  2 +-
 src/include/catalog/pg_constraint.h   | 11 +--
 src/include/commands/defrem.h |  2 +-
 src/include/nodes/parsenodes.h|  6 +-
 .../regress/expected/without_overlaps.out |  8 +--
 

Re: SQL:2011 application time

[jian he]

hi. based on v17. I found several doc related issues. previously I
didn't look closely

+ 
+  In a temporal foreign key, the delete/update will use
+  FOR PORTION OF semantics to constrain the
+  effect to the bounds being deleted/updated in the referenced row.
+ 

The first "para" should be  ?
---
There are many warnings after #define WRITE_READ_PARSE_PLAN_TREES
see: http://cfbot.cputube.org/highlights/all.html#4308
Does that mean oue new change in gram.y is somehow wrong?
--
sgml/html/sql-update.html:
"range_or_period_name
The range column or period to use when performing a temporal update.
This must match the range or period used in the table's temporal
primary key."

Is the second sentence unnecessary? since no primary can still do "for
portion of update".

sgml/html/sql-update.html:
"start_time
The earliest time (inclusive) to change in a temporal update. This
must be a value matching the base type of the range or period from
range_or_period_name. It may also be the special value MINVALUE to
indicate an update whose beginning is unbounded."

probably something like the following:
"lower_bound"
The lower bound (inclusive) to change in an overlap update. This must
be a value matching the base type of the range or period from
range_or_period_name. It may also be the special value UNBOUNDED to
indicate an update whose beginning is unbounded."

Obviously the "start_time" reference also needs to change, and the
sql-delete.html reference also needs to change.
--
UPDATE for_portion_of_test FOR PORTION OF valid_at  FROM NULL TO
"unbounded" SET name = 'NULL to NULL';
should fail, but not. double quoted unbounded is a column reference, I assume.

That's why I am confused with the function transformForPortionOfBound.
"if (nodeTag(n) == T_ColumnRef)" part.
---
in create_table.sgml. you also need to add  WITHOUT OVERLAPS related
info into 

Re: SQL:2011 application time

[jian he]

hi.

* The attached patch makes foreign keys with PERIOD fail if any of the
foreign key columns is "generated columns".

* The following queries will cause segmentation fault. not sure the
best way to fix it. the reason
in LINE: numpks = transformColumnNameList(RelationGetRelid(pkrel),
fkconstraint->pk_attrs, pkattnum, pktypoid);
begin;
drop table if exists temporal3,temporal_fk_rng2rng;
CREATE TABLE temporal3 (id int4range,valid_at tsrange,
CONSTRAINT temporal3_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS));
CREATE TABLE temporal_fk_rng2rng (
id int4range,valid_at tsrange,parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal3 (id, valid_at)
);

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

* 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);

* refactor restrict_cascading_range function.

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

anyway, I put it in one patch. please check the attached.
From 7dc2194f9bd46cfee7cfc774a2e52a2b64d465ea Mon Sep 17 00:00:00 2001
From: pgaddict 
Date: Sun, 29 Oct 2023 14:42:57 +0800
Subject: [PATCH v1 1/1] various small fixes.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* The attached patch makes foreign keys with PERIOD fail if any of the foreign key columns is "generated columns".
* change the function FindFKComparisonOperators's "eqstrategy"  to make pg_constraint record correct {conpfeqop,conppeqop,conffeqop}.
* refactor restrict_cascading_range function.
* first (is_temporal) {numfks += 1;} then (numfks != numpks) validation.
* variable comment fix.
---
 src/backend/commands/tablecmds.c| 96 -
 src/backend/utils/adt/ri_triggers.c | 32 +-
 2 files changed, 69 insertions(+), 59 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5ae0f113..86e99c81 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -522,7 +522,7 @@ static ObjectAddress addFkRecurseReferenced(List **wqueue, Constraint *fkconstra
 			bool is_temporal);
 static void validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums,
 		 int numfksetcols, const int16 *fksetcolsattnums,
-		 List *fksetcols);
+		 const int16 *fkperiodattnums, List *fksetcols);
 static void addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint,
 	Relation rel, Relation pkrel, Oid indexOid, Oid parentConstr,
 	int numfks, int16 *pkattnum, int16 *fkattnum,
@@ -10292,6 +10292,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			  fkdelsetcols, NULL);
 	validateFkOnDeleteSetColumns(numfks, fkattnum,
  numfkdelsetcols, fkdelsetcols,
+ fkperiodattnums,
  fkconstraint->fk_del_set_cols);
 
 	/*
@@ -10325,6 +10326,43 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		   opclasses);
 	}
 
+	/*
+	 * On the strength of a previous constraint, we might avoid scanning
+	 * tables to validate this one.  See below.
+	 */
+	old_check_ok = (fkconstraint->old_conpfeqop != NIL);
+	Assert(!old_check_ok || numfks == list_length(fkconstraint->old_conpfeqop));
+
+	for (i = 0; i < numpks; i++)
+	{
+		FindFKComparisonOperators(
+fkconstraint, tab, i, fkattnum,
+_check_ok, _pfeqop_item,
+pktypoid[i], fktypoid[i], opclasses[i],
+is_temporal, false,
+[i], [i], [i]);
+	}
+	/* set pfeq, ppeq, ffeq operators for withoutoverlaps constraint.
+	 * this also assume overlaps is the last key columns in constraint.
+	 *
+	*/
+	if (is_temporal)
+	{
+		pkattnum[numpks] = pkperiodattnums[0];
+		pktypoid[numpks] = pkperiodtypoids[0];
+		fkattnum[numpks] = fkperiodattnums[0];
+		fktypoid[numpks] = fkperiodtypoids[0];
+
+		FindFKComparisonOperators(
+fkconstraint, tab, numpks, fkattnum,
+_check_ok, _pfeqop_item,
+pkperiodtypoids[0], fkperiodtypoids[0], opclasses[numpks],
+is_temporal, true,
+[numpks], [numpks], [numpks]);
+		numfks += 1;
+		numpks += 1;
+	}
+
 	/*
 	 * Now we can check permissions.
 	 */
@@ -10371,38 +10409,6 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 (errcode(ERRCODE_INVALID_FOREIGN_KEY),
  errmsg("number of referencing and referenced columns for foreign key disagree")));
 
-	/*
-	 * On the strength of a previous constraint, we 

Re: SQL:2011 application time

[jian he]

V16 patch  doc/src/sgml/html/sql-createtable.html doc SET NULL description:
`
SET NULL [ ( column_name [, ... ] ) ]
Set all of the referencing columns, or a specified subset of the
referencing columns, to null. A subset of columns can only be
specified for ON DELETE actions.
In a temporal foreign key, the change will use FOR PORTION OF
semantics to constrain the effect to the bounds of the referenced row.
`

I think it means, if the foreign key has PERIOD column[s], then the
PERIOD column[s] will not be set to NULL in {ON DELETE|ON UPDATE}. We
can also use FOR PORTION OF semantics to constrain the effect to the
bounds of the referenced row.
see below demo:


BEGIN;
drop table if exists temporal_rng CASCADE;
drop table if exists temporal_fk_rng2rng CASCADE;
CREATE unlogged TABLE temporal_rng (id int4range,valid_at tsrange);
ALTER TABLE temporal_rng ADD CONSTRAINT temporal_rng_pk PRIMARY KEY
(id, valid_at WITHOUT OVERLAPS);
CREATE unlogged TABLE temporal_fk_rng2rng (id int4range,valid_at
tsrange,parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng (id, PERIOD valid_at) on update set null ON
DELETE SET NULL);

INSERT INTO temporal_rng VALUES ('[11,11]', tsrange('2018-01-01',
'2021-01-01'));
INSERT INTO temporal_fk_rng2rng VALUES ('[7,7]', tsrange('2018-01-01',
'2021-01-01'), '[11,11]');
DELETE FROM temporal_rng WHERE id = '[11,11]';
table temporal_fk_rng2rng;
commit;
-
also
"REFERENCES temporal_rng (id, PERIOD valid_at) ON UPDATE SET NULL ON
DELETE SET NULL)"
is the same as
"REFERENCES temporal_rng (id, PERIOD valid_at) ON UPDATE SET NULL ON
DELETE SET NULL (parent_id)"
in the current implementation.
we might need to change the pg_constraint column "confdelsetcols" description.
---
the above also applies to SET DEFAULT.

--
can you add the following for the sake of code coverage. I think
src/test/regress/sql/without_overlaps.sql can be simplified.

--- common template for test foreign key constraint.
CREATE OR REPLACE PROCEDURE overlap_template()
LANGUAGE SQL
AS $$
DROP TABLE IF EXISTS temporal_rng CASCADE;
DROP TABLE IF EXISTS temporal_fk_rng2rng CASCADE;
CREATE UNLOGGED TABLE temporal_rng (id int4range,valid_at tsrange);
ALTER TABLE temporal_rng
ADD CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS);
CREATE UNLOGGED TABLE temporal_fk_rng2rng (
id int4range,
valid_at tsrange,
parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng (id, PERIOD valid_at)
ON UPDATE no action ON DELETE no action
DEFERRABLE
);
$$;
call overlap_template();

--- on update/delete restrict
-- coverage for TRI_FKey_restrict_upd,TRI_FKey_restrict_del.
BEGIN;
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(id,PERIOD valid_at) ON UPDATE RESTRICT ON
DELETE RESTRICT;

INSERT INTO temporal_rng VALUES ('[11,11]', tsrange('2018-01-01',
'2021-01-01'));
INSERT INTO temporal_fk_rng2rng VALUES ('[7,7]', tsrange('2018-01-01',
'2020-01-01'), '[11,11]');
savepoint s;

UPDATE temporal_rng FOR PORTION OF valid_at FROM '2018-01-01' TO '2018-01-03'
SET id = '[9,9]' WHERE id = '[11,11]';
ROLLBACK to s;
delete from  temporal_rng FOR PORTION OF valid_at FROM '2018-01-01' TO
'2020-01-01';
ROLLBACK to s;
--this one should not have error.
delete from  temporal_rng FOR PORTION OF valid_at FROM '2020-01-01' TO
'2021-01-01';
table temporal_rng;
ROLLBACK;

-
--- on delete set column list coverage for function tri_set. branch
{if (riinfo->ndelsetcols != 0)}
BEGIN;
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(id,PERIOD valid_at) ON DELETE set default(parent_id);

ALTER TABLE temporal_fk_rng2rng  ALTER COLUMN parent_id SET DEFAULT '[2,2]';
ALTER TABLE temporal_fk_rng2rng  ALTER COLUMN valid_at SET DEFAULT tsrange'(,)';
INSERT INTO temporal_rng VALUES ('[11,11]', tsrange('2018-01-01',
'2021-01-01'));
INSERT INTO temporal_fk_rng2rng VALUES ('[7,7]', tsrange('2018-01-01',
'2020-01-01'), '[11,11]');
insert into temporal_rng values('[2,2]','(,)');
savepoint s;
delete from  temporal_rng FOR PORTION OF valid_at FROM '2018-01-01' TO
'2019-01-01' where id = '[11,11]';
-- delete from  temporal_rng where id = '[11,11]';
table temporal_fk_rng2rng;
rollback;

Re: SQL:2011 application time

[jian he]

On Wed, Oct 11, 2023 at 12:47 PM Paul Jungwirth
 wrote:
>
> On 9/25/23 14:00, Peter Eisentraut wrote:
> > Looking through the tests in v16-0001:
> >
> > +-- PK with no columns just WITHOUT OVERLAPS:
> > +CREATE TABLE temporal_rng (
> > +   valid_at tsrange,
> > +   CONSTRAINT temporal_rng_pk PRIMARY KEY (valid_at WITHOUT OVERLAPS)
> > +);
> > +ERROR:  syntax error at or near "WITHOUT"
> > +LINE 3:  CONSTRAINT temporal_rng_pk PRIMARY KEY (valid_at WITHOUT OV...
> > +  ^
> >
> > I think this error is confusing.  The SQL standard requires at least one
> > non-period column in a PK.  I don't know why that is or why we should
> > implement it.  But if we want to implement it, maybe we should enforce
> > that in parse analysis rather than directly in the parser, to be able to
> > produce a more friendly error message.
>
> Okay.
>
> (I think the reason the standard requires one non-period column is to
> identify the "entity". If philosophically the row is an Aristotelian
> proposition about that thing, the period qualifies it as true just
> during some time span. So the scalar part is doing the work that a PK
> conventionally does, and the period part does something else. Perhaps a
> PK/UNIQUE constraint with no scalar part would still be useful, but not
> very often I think, and I'm not sure it makes sense to call it PK/UNIQUE.)
>
> > +-- PK with a range column/PERIOD that isn't there:
> > +CREATE TABLE temporal_rng (
> > +   id INTEGER,
> > +   CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT
> > OVERLAPS)
> > +);
> > +ERROR:  range or PERIOD "valid_at" in WITHOUT OVERLAPS does not exist
> >
> > I think here we should just produce a "column doesn't exist" error
> > message, the same as if the "id" column was invalid.  We don't need to
> > get into the details of what kind of column it should be.  That is done
> > in the next test
>
> I'll change it. The reason for the different wording is that it might
> not be a column at all. It might be a PERIOD. So what about just "column
> or PERIOD doesn't exist"? (Your suggestion is fine too though.)
>
> > +ERROR:  column "valid_at" in WITHOUT OVERLAPS is not a range type
> >
> > Also, in any case it would be nice to have a location pointer here (for
> > both cases).
>
> Agreed.
>

I refactored findNeworOldColumn to better handle error reports.
please check the attached.
From 81a5eab2596c0e322dee8165ad73343328f496ed Mon Sep 17 00:00:00 2001
From: pgaddict 
Date: Wed, 25 Oct 2023 13:50:38 +0800
Subject: [PATCH v1 1/1] refactor findNewOrOldColumn.

refator findNeworOldColumn to validate WITHOUT OVERLAP constraint
associated key column exists and it's a range data type.

create/alter table add without overlap constraint:
if the key column not exists,
it will report the same as no "without overlap" normal case.

create table add without overlap constraint:
if overlap constraint associated column's data type is not range,
it will report it's not a range error, also print out the
parser position.

alter table add without overlap constraint:
if overlap constraint associated column's data type is not range,
it will report not a range data type error.
it will not print out parser position (I don't know how to do that).
---
 src/backend/parser/parse_utilcmd.c| 81 ++-
 .../regress/expected/without_overlaps.out |  8 +-
 2 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index e195410c..f44de591 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -124,8 +124,7 @@ static List *get_opclass(Oid opclass, Oid actual_datatype);
 static void transformIndexConstraints(CreateStmtContext *cxt);
 static IndexStmt *transformIndexConstraint(Constraint *constraint,
 		   CreateStmtContext *cxt);
-static bool findNewOrOldColumn(CreateStmtContext *cxt, char *colname, char **typname,
-			   Oid *typid);
+static void validate_without_overlaps(CreateStmtContext *cxt, char *colname);
 static void transformExtendedStatistics(CreateStmtContext *cxt);
 static void transformFKConstraints(CreateStmtContext *cxt,
    bool skipValidation,
@@ -2698,34 +2697,19 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
 		 * Anything in without_overlaps should be included,
 		 * but with the overlaps operator (&&) instead of equality.
 		 */
-		if (constraint->without_overlaps != NULL) {
+		if (constraint->without_overlaps != NULL)
+		{
 			char *without_overlaps_str = strVal(constraint->without_overlaps);
 			IndexElem *iparam = makeNode(IndexElem);
-			char   *typname;
-			Oid		typid;
 
 			/*
 			 * Iterate through the table's columns
 			 * (like just a little bit above).
 			 * If we find one whose name is the same as without_overlaps,
 			 * validate that it's a range type.
-			 *
-			 * Otherwise report an error.
+			 * if it 

Re: SQL:2011 application time

[jian he]

hi. also based on v16.
-tests.
drop table if exists for_portion_of_test1;
CREATE unlogged TABLE for_portion_of_test1 (id int4range, valid_at
tsrange,name text );
INSERT INTO for_portion_of_test1 VALUES  ('[1,1]', NULL,
'[1,1]_NULL'),('[1,1]', '(,)', '()_[1,]')
,('[1,1]', 'empty', '[1,1]_empty'),(NULL,NULL, NULL), (nuLL,
'(2018-01-01,2019-01-01)','misc');
--1
UPDATE for_portion_of_test1 FOR PORTION OF valid_at FROM NULL TO NULL
SET name = 'for_portition_NULLtoNULL';
select * from for_portion_of_test1;
--2
UPDATE for_portion_of_test1 FOR PORTION OF valid_at FROM null TO
UNBOUNDED SET name = 'NULL_TO_UNBOUNDED';
select * from for_portion_of_test1;
--3
UPDATE for_portion_of_test1 FOR PORTION OF valid_at FROM UNBOUNDED TO
null SET name = 'UNBOUNDED__TO_NULL';
select * from for_portion_of_test1;
--4
UPDATE for_portion_of_test1 FOR PORTION OF valid_at FROM UNBOUNDED TO
UNBOUNDED SET name = 'UNBOUNDED__TO_UNBOUNDED';
select * from for_portion_of_test1;

File: /src/backend/executor/nodeModifyTable.c
1277: oldRange = slot_getattr(oldtupleSlot,
forPortionOf->rangeVar->varattno, );
1278:
1279: if (isNull)
1280: elog(ERROR, "found a NULL range in a temporal table");
1281: oldRangeType = DatumGetRangeTypeP(oldRange);

I wonder when this isNull will be invoked. the above tests won't
invoke the error.
also the above test, NULL seems equivalent to unbounded. FOR PORTION
OF "from" and "to" both bound should not be null?

which means the following code does not work as intended? I also
cannot find a way to invoke the following elog error branch.
File:src/backend/executor/nodeModifyTable.c
4458: exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange, estate);
4459: targetRange = ExecEvalExpr(exprState, econtext, );
4460: if (isNull)
4461: elog(ERROR, "Got a NULL FOR PORTION OF target range");

---
i also made some changes in the function range_leftover_internal,
ExecForPortionOfLeftovers.
please see the attached patch.
From 5cdd99f9a63d381985541b0df539c2bb92ef7276 Mon Sep 17 00:00:00 2001
From: pgaddict 
Date: Sat, 21 Oct 2023 17:18:50 +0800
Subject: [PATCH v1 1/1] refactor function range_leftover_internal

renaming variable.
better comment.

---
 src/backend/utils/adt/rangetypes.c | 55 +-
 src/include/utils/rangetypes.h |  5 ++-
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/src/backend/utils/adt/rangetypes.c b/src/backend/utils/adt/rangetypes.c
index d4e17323..a5a34450 100644
--- a/src/backend/utils/adt/rangetypes.c
+++ b/src/backend/utils/adt/rangetypes.c
@@ -1207,47 +1207,54 @@ range_split_internal(TypeCacheEntry *typcache, const RangeType *r1, const RangeT
 }
 
 /*
- * range_leftover_internal - Sets output1 and output2 to the remaining parts of r1
- * after subtracting r2, or if nothing is left then to the empty range.
- * output1 will always be "before" r2 and output2 "after".
+ * Sets leftoverRange and rightoverRange to the remaining parts of oldRange
+ * after subtracting targetRange, or if nothing is left then to the empty range.
+ * leftoverRange will always be "before" targetRange and rightoverRange "after" targetRange.
  */
 void
-range_leftover_internal(TypeCacheEntry *typcache, const RangeType *r1,
-		const RangeType *r2, RangeType **output1, RangeType **output2)
+range_leftover_internal(TypeCacheEntry *typcache, const RangeType *oldRange,
+		const RangeType *targetRange, RangeType **leftoverRange, RangeType **rightoverRange)
 {
-	RangeBound	lower1,
-lower2;
-	RangeBound	upper1,
-upper2;
-	bool		empty1,
-empty2;
+	RangeBound	lower_oldRange,
+lower_targetRange;
+	RangeBound	upper_oldRange,
+upper_targetRange;
+	bool		oldRange_is_empty,
+targetRange_is_empty;
 
-	range_deserialize(typcache, r1, , , );
-	range_deserialize(typcache, r2, , , );
+	range_deserialize(typcache, oldRange, _oldRange, _oldRange, _is_empty);
+	range_deserialize(typcache, targetRange, _targetRange, _targetRange, _is_empty);
 
-	if (range_cmp_bounds(typcache, , ) < 0)
+	/*
+	* FOR PORTION OF. upper_targetRange, if oldRange do not overlaps with targetRangeType.
+	* So these two range must overlaps, that means both range should not be empty.
+	*
+	*/
+	Assert(!oldRange_is_empty);
+	Assert(!targetRange_is_empty);
+
+	if (range_cmp_bounds(typcache, _oldRange, _targetRange) < 0)
 	{
-		lower2.inclusive = !lower2.inclusive;
-		lower2.lower = false;
-		*output1 = make_range(typcache, , , false, NULL);
+		lower_targetRange.inclusive = !lower_targetRange.inclusive;
+		lower_targetRange.lower = false;
+		*leftoverRange = make_range(typcache, _oldRange, _targetRange, false, NULL);
 	}
 	else
 	{
-		*output1 = make_empty_range(typcache);
+		*leftoverRange = make_empty_range(typcache);
 	}
 
-	if (range_cmp_bounds(typcache, , ) > 0)
+	if (range_cmp_bounds(typcache, _oldRange, _targetRange) > 0)
 	{
-		upper2.inclusive = !upper2.inclusive;
-		upper2.lower = true;
-		*output2 = make_range(typcache, , , 

Re: SQL:2011 application time

[jian he]

Hi.
based on v16.

/* Look up the FOR PORTION OF name requested. */
range_attno = attnameAttNum(targetrel, range_name, false);
if (range_attno == InvalidAttrNumber)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column or period \"%s\" of relation \"%s\" does not exist",
range_name,
RelationGetRelationName(targetrel)),
parser_errposition(pstate, forPortionOf->range_name_location)));
attr = TupleDescAttr(targetrel->rd_att, range_attno - 1);
// TODO: check attr->attisdropped (?),
// and figure out concurrency issues with that in general.
// It should work the same as updating any other column.

I don't think we need to check attr->attisdropped here.
because the above function attnameAttNum already does the job.

bool
get_typname_and_namespace(Oid typid, char **typname, char **typnamespace)
{
HeapTuple tp;

tp = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typid));
if (HeapTupleIsValid(tp))
{
Form_pg_type typtup = (Form_pg_type) GETSTRUCT(tp);

*typname = pstrdup(NameStr(typtup->typname));
*typnamespace = get_namespace_name(typtup->typnamespace);
ReleaseSysCache(tp);
return *typnamespace;

"return *typnamespace;" should be "return true"?
Maybe name it  to get_typname_and_typnamespace?
---
if (!get_typname_and_namespace(attr->atttypid, _type_name,
_type_namespace))
elog(ERROR, "missing range type %d", attr->atttypid);

you can just `elog(ERROR, "missing range type %s", range_type_name);` ?
Also, this should be placed just below if (!type_is_range(attr->atttypid))?
---
src/backend/catalog/objectaddress.c

if (OidIsValid(per->perrelid))
{
StringInfoData rel;

initStringInfo();
getRelationDescription(, per->perrelid, false);
appendStringInfo(, _("period %s on %s"),
NameStr(per->pername), rel.data);
pfree(rel.data);
}
else
{
appendStringInfo(, _("period %s"),
NameStr(per->pername));
}

periods are always associated with the table, is the above else branch correct?
---
File: src/backend/commands/tablecmds.c
7899: /*
7900: * this test is deliberately not attisdropped-aware, since if one tries to
7901: * add a column matching a dropped column name, it's gonna fail anyway.
7902: *
7903: * XXX: Does this hold for periods?
7904: */
7905: attTuple = SearchSysCache2(ATTNAME,
7906:ObjectIdGetDatum(RelationGetRelid(rel)),
7907:PointerGetDatum(pername));

XXX: Does this hold for periods?
Yes. we can add the following 2 sql for code coverage.
alter table pt add period for tableoid (ds, de);
alter table pt add period for "pg.dropped.4" (ds, de);

Re: SQL:2011 application time

[jian he]

On Tue, Sep 26, 2023 at 4:21 AM Paul Jungwirth
 wrote:
>
> On 9/24/23 21:52, jian he wrote:
> > On Wed, Sep 20, 2023 at 10:50 AM Paul Jungwirth
> >  wrote:
> >>
> >> 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.
> >>
> >
> > hi. some tiny issues.
>
> Rebased v16 patches attached.

Can you rebase it?
changes in
https://git.postgresql.org/cgit/postgresql.git/log/src/backend/executor/nodeModifyTable.c
https://git.postgresql.org/cgit/postgresql.git/log/src/backend/commands/tablecmds.c
make it  no longer applicable.

I try to manually edit the patch to make it applicable.
but failed at tablecmds.c

Re: SQL:2011 application time

[Vik Fearing]

On 10/11/23 05:47, Paul Jungwirth wrote:
+SELECT pg_get_indexdef(conindid, 0, true) FROM pg_constraint WHERE 
conname = 'temporal_rng_pk';

+    pg_get_indexdef
+---
+ CREATE UNIQUE INDEX temporal_rng_pk ON temporal_rng USING gist (id, 
valid_at)


Shouldn't this somehow show the operator classes for the columns?  We 
are using different operator classes for the id and valid_at columns, 
aren't we?


We only print the operator classes if they are not the default, so they 
don't appear here.


I do suspect something more is desirable though. For exclusion 
constraints we replace everything before the columns with just "EXCLUDE 
USING gist". I could embed WITHOUT OVERLAPS but it's not valid syntax in 
CREATE INDEX. Let me know if you have any ideas.


Why not?  The standard does not mention indexes (although some 
discussions last week might change that) so we can change the syntax for 
it as we wish.  Doing so would also allow us to use ALTER TABLE ... 
USING INDEX for such things.

--
Vik Fearing

Re: SQL:2011 application time

[Paul Jungwirth]

On 9/25/23 14:00, Peter Eisentraut wrote:

Looking through the tests in v16-0001:

+-- PK with no columns just WITHOUT OVERLAPS:
+CREATE TABLE temporal_rng (
+   valid_at tsrange,
+   CONSTRAINT temporal_rng_pk PRIMARY KEY (valid_at WITHOUT OVERLAPS)
+);
+ERROR:  syntax error at or near "WITHOUT"
+LINE 3:  CONSTRAINT temporal_rng_pk PRIMARY KEY (valid_at WITHOUT OV...
+  ^

I think this error is confusing.  The SQL standard requires at least one 
non-period column in a PK.  I don't know why that is or why we should 
implement it.  But if we want to implement it, maybe we should enforce 
that in parse analysis rather than directly in the parser, to be able to 
produce a more friendly error message.


Okay.

(I think the reason the standard requires one non-period column is to 
identify the "entity". If philosophically the row is an Aristotelian 
proposition about that thing, the period qualifies it as true just 
during some time span. So the scalar part is doing the work that a PK 
conventionally does, and the period part does something else. Perhaps a 
PK/UNIQUE constraint with no scalar part would still be useful, but not 
very often I think, and I'm not sure it makes sense to call it PK/UNIQUE.)



+-- PK with a range column/PERIOD that isn't there:
+CREATE TABLE temporal_rng (
+   id INTEGER,
+   CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT 
OVERLAPS)

+);
+ERROR:  range or PERIOD "valid_at" in WITHOUT OVERLAPS does not exist

I think here we should just produce a "column doesn't exist" error 
message, the same as if the "id" column was invalid.  We don't need to 
get into the details of what kind of column it should be.  That is done 
in the next test


I'll change it. The reason for the different wording is that it might 
not be a column at all. It might be a PERIOD. So what about just "column 
or PERIOD doesn't exist"? (Your suggestion is fine too though.)



+ERROR:  column "valid_at" in WITHOUT OVERLAPS is not a range type

Also, in any case it would be nice to have a location pointer here (for 
both cases).


Agreed.


+-- PK with one column plus a range:
+CREATE TABLE temporal_rng (
+   -- Since we can't depend on having btree_gist here,
+   -- use an int4range instead of an int.
+   -- (The rangetypes regression test uses the same trick.)
+   id int4range,
+   valid_at tsrange,
+   CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT 
OVERLAPS)

+);

I'm confused why you are using int4range here (and in further tests) for 
the scalar (non-range) part of the primary key.  Wouldn't a plaint int4 
serve here?


A plain int4 would be better, and it would match the normal use-case, 
but you must have btree_gist to create an index like that, and the 
regress tests can't assume we have that. Here is the part from 
sql/rangetypes.sql I'm referring to:


--
-- Btree_gist is not included by default, so to test exclusion
-- constraints with range types, use singleton int ranges for the "="
-- portion of the constraint.
--

create table test_range_excl(
  room int4range,
  speaker int4range,
  during tsrange,
  exclude using gist (room with =, during with &&),
  exclude using gist (speaker with =, during with &&)
);

+SELECT pg_get_indexdef(conindid, 0, true) FROM pg_constraint WHERE 
conname = 'temporal_rng_pk';

+    pg_get_indexdef
+---
+ CREATE UNIQUE INDEX temporal_rng_pk ON temporal_rng USING gist (id, 
valid_at)


Shouldn't this somehow show the operator classes for the columns?  We 
are using different operator classes for the id and valid_at columns, 
aren't we?


We only print the operator classes if they are not the default, so they 
don't appear here.


I do suspect something more is desirable though. For exclusion 
constraints we replace everything before the columns with just "EXCLUDE 
USING gist". I could embed WITHOUT OVERLAPS but it's not valid syntax in 
CREATE INDEX. Let me know if you have any ideas.



+-- PK with USING INDEX (not possible):
+CREATE TABLE temporal3 (
+   id int4range,
+   valid_at tsrange
+);
+CREATE INDEX idx_temporal3_uq ON temporal3 USING gist (id, valid_at);
+ALTER TABLE temporal3
+   ADD CONSTRAINT temporal3_pk
+   PRIMARY KEY USING INDEX idx_temporal3_uq;
+ERROR:  "idx_temporal3_uq" is not a unique index
+LINE 2:  ADD CONSTRAINT temporal3_pk
+ ^
+DETAIL:  Cannot create a primary key or unique constraint using such an 
index.


Could you also add a test where the index is unique and the whole thing 
does work?


No problem!

Apart from the tests, how about renaming the column 
pg_constraint.contemporal to something like to conwithoutoverlaps?


Is that too verbose? I've got some code already changing it to 
conoverlaps but I'm probably happier with conwithoutoverlaps, assuming 
no one else minds it.


Yours,

--

Re: SQL:2011 application time

[Paul Jungwirth]

Hi Peter et al,

On 9/1/23 12:56, Paul Jungwirth wrote:

On 9/1/23 11:30, Peter Eisentraut wrote:
I think the WITHOUT OVERLAPS clause should be per-column, so that 
something like UNIQUE (a WITHOUT OVERLAPS, b, c WITHOUT OVERLAPS) 
would be possible.  Then the WITHOUT OVERLAPS clause would directly 
correspond to the choice between equality or overlaps operator per 
column.
I think allowing multiple uses of `WITHOUT OVERLAPS` (and in any 
position) is a great recommendation that enables a lot of new 
functionality.


I've been working on implementing this, but I've come to think it is the 
wrong way to go.


If we support this in primary key and unique constraints, then we must 
also support it for foreign keys and UPDATE/DELETE FOR PORTION OF. But 
implementing that logic is pretty tricky. For example take a foreign key 
on (id, PERIOD valid_at, PERIOD asserted_at). We need to ensure the 
referenced two-dimensional time space `contains` the referencing 
two-dimensional space. You can visualize a rectangle in two-dimensional 
space for each referencing record (which we validate one at a time). The 
referenced records must be aggregated and so form a polygon (of all 
right angles). For example the referencing record may be (1, [0,2), 
[0,2)) with referenced records of (1, [0,2), [0,1)) and (1, [0,1), 
[1,2)). (I'm using intranges since they're easier to read, but you could 
imagine these as dateranges like [2000-01-01,2002-01-01).) Now the 
range_agg of their valid_ats is [0,2) and of their asserted_ats is 
[0,2). But the referenced 2d space still doesn't contain the referencing 
space. It's got one corner missing. This is a well-known problem among 
game developers. We're lucky not to have arbitrary polygons, but it's 
still a tough issue.


Besides `contains` we also need to compute `overlaps` and `intersects` 
to support these temporal features. Implementing that for 2d, 3d, etc 
looks very complicated, for something that is far outside the normal use 
case and also not part of the standard. It will cost a little 
performance for the normal 1d use case too.


I think a better approach (which I want to attempt as an add-on patch, 
not in this main series) is to support not just range types, but any 
type with the necessary operators. Then you could have an mdrange 
(multi-dimensional range) or potentially even an arbitrary n-dimensional 
polygon. (PostGIS has something like this, but its `contains` operator 
compares (non-concave) *bounding boxes*, so it would not work for the 
example above. Still the similarity between temporal and spatial data is 
striking. I'm going to see if I can get some input from PostGIS folks 
about how useful any of this is to them.) This approach would also let 
us use multiranges: not for multiple dimensions, but for non-contiguous 
time spans stored in a single row. This puts the complexity in the types 
themselves (which seems more appropriate) and is ultimately more 
flexible (supporting not just mdrange but also multirange, and other 
things too).


This approach also means that instead of storing a mask/list of which 
columns use WITHOUT OVERLAPS, I can just store one attnum. Again, this 
saves the common use-case from paying a performance penalty to support a 
much rarer one.


I've still got my multi-WITHOUT OVERLAPS work, but I'm going to switch 
gears to what I've described here. Please let me know if you disagree!


Yours,

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

Re: SQL:2011 application time

[Peter Eisentraut]

On 25.09.23 21:20, Paul Jungwirth wrote:

On 9/24/23 21:52, jian he wrote:

On Wed, Sep 20, 2023 at 10:50 AM Paul Jungwirth
 wrote:


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.



hi. some tiny issues.


Rebased v16 patches attached.


Looking through the tests in v16-0001:

+-- PK with no columns just WITHOUT OVERLAPS:
+CREATE TABLE temporal_rng (
+   valid_at tsrange,
+   CONSTRAINT temporal_rng_pk PRIMARY KEY (valid_at WITHOUT OVERLAPS)
+);
+ERROR:  syntax error at or near "WITHOUT"
+LINE 3:  CONSTRAINT temporal_rng_pk PRIMARY KEY (valid_at WITHOUT OV...
+  ^

I think this error is confusing.  The SQL standard requires at least one 
non-period column in a PK.  I don't know why that is or why we should 
implement it.  But if we want to implement it, maybe we should enforce 
that in parse analysis rather than directly in the parser, to be able to 
produce a more friendly error message.


+-- PK with a range column/PERIOD that isn't there:
+CREATE TABLE temporal_rng (
+   id INTEGER,
+   CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT 
OVERLAPS)

+);
+ERROR:  range or PERIOD "valid_at" in WITHOUT OVERLAPS does not exist

I think here we should just produce a "column doesn't exist" error 
message, the same as if the "id" column was invalid.  We don't need to 
get into the details of what kind of column it should be.  That is done 
in the next test


+ERROR:  column "valid_at" in WITHOUT OVERLAPS is not a range type

Also, in any case it would be nice to have a location pointer here (for 
both cases).


+-- PK with one column plus a range:
+CREATE TABLE temporal_rng (
+   -- Since we can't depend on having btree_gist here,
+   -- use an int4range instead of an int.
+   -- (The rangetypes regression test uses the same trick.)
+   id int4range,
+   valid_at tsrange,
+   CONSTRAINT temporal_rng_pk PRIMARY KEY (id, valid_at WITHOUT 
OVERLAPS)

+);

I'm confused why you are using int4range here (and in further tests) for 
the scalar (non-range) part of the primary key.  Wouldn't a plaint int4 
serve here?


+SELECT pg_get_indexdef(conindid, 0, true) FROM pg_constraint WHERE 
conname = 'temporal_rng_pk';

+pg_get_indexdef
+---
+ CREATE UNIQUE INDEX temporal_rng_pk ON temporal_rng USING gist (id, 
valid_at)


Shouldn't this somehow show the operator classes for the columns?  We 
are using different operator classes for the id and valid_at columns, 
aren't we?


+-- PK with USING INDEX (not possible):
+CREATE TABLE temporal3 (
+   id int4range,
+   valid_at tsrange
+);
+CREATE INDEX idx_temporal3_uq ON temporal3 USING gist (id, valid_at);
+ALTER TABLE temporal3
+   ADD CONSTRAINT temporal3_pk
+   PRIMARY KEY USING INDEX idx_temporal3_uq;
+ERROR:  "idx_temporal3_uq" is not a unique index
+LINE 2:  ADD CONSTRAINT temporal3_pk
+ ^
+DETAIL:  Cannot create a primary key or unique constraint using such an 
index.


Could you also add a test where the index is unique and the whole thing 
does work?



Apart from the tests, how about renaming the column 
pg_constraint.contemporal to something like to conwithoutoverlaps?

Re: SQL:2011 application time

[jian he]

On Wed, Sep 20, 2023 at 10:50 AM Paul Jungwirth
 wrote:
>
> 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.
>

hi. some tiny issues.
IN src/backend/utils/adt/ri_triggers.c

else {
appendStringInfo(, "SELECT 1 FROM %s%s x",
pk_only, pkrelname);
}
should change to

else
{
appendStringInfo(, "SELECT 1 FROM %s%s x",
pk_only, pkrelname);
}


It would be better, we mention it somewhere:
by default, you can only have a primary key(range_type[...],
range_type WITHOUT OVERLAPS).

preceding without overlaps, all columns (in primary key) data types
only allowed range types.
---
The WITHOUT OVERLAPS value must be a range type and is used to
constrain the record's applicability to just that interval (usually a
range of dates or timestamps).

"interval", I think "period" or "range" would be better. I am not sure
we need to mention " must be a range type, not a multi range type".
-
I just `git apply`, then ran the test, and one test failed. Some minor
changes need to make the test pass.

Re: SQL:2011 application time

[jian he]

On Fri, Sep 15, 2023 at 12:11 AM Paul Jungwirth
 wrote:
>
> 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).
>

another case:
BEGIN;
DROP TABLE IF EXISTS temporal_rng, temporal_fk_rng2rng;
CREATE TABLE temporal_rng ( id int4range,valid_at tsrange);
ALTER TABLE temporal_rng
ADD CONSTRAINT temporal_rng_pk
PRIMARY KEY (id, valid_at WITHOUT OVERLAPS);
CREATE TABLE temporal_fk_rng2rng (
id int4range,
valid_at tsrange,
parent_id int4range
);
INSERT INTO temporal_rng VALUES ('[5,5]', tsrange('2018-01-01', '2018-02-01')),
 ('[5,5]', tsrange('2018-02-01', '2018-03-01'));
INSERT INTO temporal_fk_rng2rng
VALUES ('[3,3]', tsrange('2018-01-05','2018-01-10'), '[5,5]');
commit;


BEGIN;
ALTER TABLE temporal_fk_rng2rng
DROP CONSTRAINT IF EXISTS temporal_fk_rng2rng_fk;
ALTER TABLE temporal_fk_rng2rng
ADD CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng
ON DELETE NO ACTION
ON UPDATE NO ACTION;
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.

---
seems in ATExecAlterConstrRecurse change to

/*
* Update deferrability of RI_FKey_noaction_del,
* RI_FKey_noaction_upd, RI_FKey_check_ins and RI_FKey_check_upd
* triggers, but not others; see createForeignKeyActionTriggers
* and CreateFKCheckTrigger.
*/
if (tgform->tgfoid != F_RI_FKEY_NOACTION_DEL &&
tgform->tgfoid != F_TRI_FKEY_NOACTION_DEL &&
tgform->tgfoid != F_RI_FKEY_NOACTION_UPD &&
tgform->tgfoid != F_TRI_FKEY_NOACTION_UPD &&
tgform->tgfoid != F_RI_FKEY_CHECK_INS &&
tgform->tgfoid != F_TRI_FKEY_CHECK_INS &&
tgform->tgfoid != F_RI_FKEY_CHECK_UPD &&
tgform->tgfoid != F_TRI_FKEY_CHECK_UPD)
continue;

will work.

Re: SQL:2011 application time

[jian he]

On Fri, Sep 15, 2023 at 12:11 AM Paul Jungwirth
 wrote:
>
>
> 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.
>

small issues so far I found, v14.

IndexInfo struct definition comment still has Temporal related
comment, should be removed.

catalog-pg-index.html, no indperiod doc entry, also in table pg_index,
column indperiod  is junk value now.
I think in UpdateIndexRelation, you need an add indperiod to build a
pg_index tuple, similar to what you did in CreateConstraintEntry.

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));


/* 
 * 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. 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


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
FROMinformation_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?

Re: SQL:2011 application time

[Paul Jungwirth]

On 9/7/23 18:24, jian he wrote:

for a range primary key, is it fine to expect it to be unique, not
null and also not overlap? (i am not sure how hard to implement it).

-
quote from 7IWD2-02-Foundation-2011-12.pdf. 4.18.3.2 Unique
constraints, page 97 of 1483.

...
-
based on the above, the unique constraint does not specify that the
column list must be range type. UNIQUE (a, c WITHOUT OVERLAPS).
Here column "a" can be a range type (that have overlap property) and
can be not.
In fact, many of your primary key, foreign key regess test using
something like '[11,11]' (which make it more easy to understand),
which in logic is a non-range usage.
So UNIQUE (a, c WITHOUT OVERLAPS), column "a" be a non-range data type
does make sense?


I'm not sure I understand this question, but here are a few things that 
might help clarify things:


In SQL:2011, a temporal primary key, unique constraint, or foreign key 
may have one or more "scalar" parts (just like a regular key) followed 
by one "PERIOD" part, which is denoted with "WITHOUT OVERLAPS" (in 
PKs/UNIQUEs) or "PERIOD" (in FKs). Except for this last key part, 
everything is still compared for equality, just as in a traditional key. 
But this last part is compared for overlaps. It's exactly the same as 
`EXCLUDE (id WITH =, valid_at WITH &&)`. The overlap part must come last 
and you can have only one (but you may have more than one scalar part if 
you like).


In the patch, I have followed that pattern, except I also allow a 
regular range column anywhere I allow a PERIOD. In fact PERIODs are 
mostly implemented on top of range types. (Until recently PERIOD support 
was in the first patch, not the last, and there was code all throughout 
for handling both, e.g. within indexes, etc. But at pgcon Peter 
suggested building everything on just range columns, and then having 
PERIODs create an "internal" GENERATED column, and that cleaned up the 
code considerably.)


One possible source of confusion is that in the tests I'm using range 
columns *also* for the scalar key part. So valid_at is a tsrange, and 
int is an int4range. This is not normally how you'd use the feature, but 
you need the btree_gist extension to mix int & tsrange (e.g.), and 
that's not available in the regress tests. We are still comparing the 
int4range for regular equality and the tsrange for overlaps. If you 
search this thread there was some discussion about bringing btree_gist 
into core, but it sounds like it doesn't need to happen. (It might be 
still desirable independently. EXCLUDE constraints are also not really 
something you can use practically without it, and their tests use the 
same trick of comparing ranges for plain equality.)


The piece of discussion you're replying to is about allowing *multiple* 
WITHOUT OVERLAPS modifiers on a PK/UNIQUE constraint, and in any 
position. I think that's a good idea, so I've started adapting the code 
to support it. (In fact there is a lot of code that assumes the overlaps 
key part will be in the last position, and I've never really been happy 
with that, so it's an excuse to make that more robust.) Here I'm saying 
(1) you will still need at least one scalar key part, (2) if there are 
no WITHOUT OVERLAPS parts then you just have a regular key, not a 
temporal one, (3) changing this obliges us to do the same for foreign 
keys and FOR PORTION OF.


I hope that helps! I apologize if I've completely missed the point. If 
so please try again. :-)


Yours,

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

Re: SQL:2011 application time

[jian he]

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?
-
ERROR:  column "valid_at" named in WITHOUT OVERLAPS is not a range type

IMHO, "named" is unnecessary.
-
doc/src/sgml/catalogs.sgml
pg_constraint adds another attribute (column): contemporal, seems no doc entry.

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

Re: SQL:2011 application time

[jian he]

hi
I am confused by (pk,fk) on delete on update (restriction and no
action) result based on v13.
related post: 
https://stackoverflow.com/questions/14921668/difference-between-restrict-and-no-action
Please check the following test and comments.

---common setup for test0, test1,test2,test3
BEGIN;
DROP TABLE IF EXISTS temporal_rng, temporal_fk_rng2rng;
CREATE TABLE temporal_rng ( id int4range,valid_at tsrange);
ALTER TABLE temporal_rng
ADD CONSTRAINT temporal_rng_pk
PRIMARY KEY (id, valid_at WITHOUT OVERLAPS);
CREATE TABLE temporal_fk_rng2rng (
id int4range,
valid_at tsrange,
parent_id int4range
);
commit;

no_action_vs_restriction test0
BEGIN;
ALTER TABLE temporal_fk_rng2rng
DROP CONSTRAINT IF EXISTS temporal_fk_rng2rng_fk;
ALTER TABLE temporal_fk_rng2rng
ADD CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng
ON DELETE NO ACTION
ON UPDATE NO ACTION;
INSERT INTO temporal_rng VALUES ('[5,5]', tsrange('2018-01-01', '2018-02-01')),
 ('[5,5]', tsrange('2018-02-01', '2018-03-01'));
INSERT INTO temporal_fk_rng2rng VALUES ('[3,3]', tsrange('2018-01-05',
'2018-01-10'), '[5,5]');

/*
expect below to fail.
since to be deleted range is being referenced (in temporal_fk_rng2rng)
but the v13 patch won't fail.
*/
delete from temporal_rng
FOR PORTION OF valid_at FROM '2018-01-06' TO '2018-01-11'
WHERE   id = '[5,5]'
AND valid_at @> '2018-01-05'::timestamp;
TABLE temporal_rng \; table temporal_fk_rng2rng;
ROLLBACK;


no_action_vs_restriction test1
BEGIN;
ALTER TABLE temporal_fk_rng2rng
DROP CONSTRAINT IF EXISTS temporal_fk_rng2rng_fk;
ALTER TABLE temporal_fk_rng2rng
ADD CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng
ON DELETE RESTRICT
ON UPDATE RESTRICT;
INSERT INTO temporal_rng VALUES ('[5,5]', tsrange('2018-01-01', '2018-02-01')),
 ('[5,5]', tsrange('2018-02-01', '2018-03-01'));
INSERT INTO temporal_fk_rng2rng VALUES ('[3,3]', tsrange('2018-01-05',
'2018-01-10'), '[5,5]');

/*
expect the below command not to fail.
since to be deleted range is not being referenced in temporal_fk_rng2rng)
but the v13 patch will fail.
*/
delete from temporal_rng
FOR PORTION OF valid_at FROM '2018-01-12' TO '2018-01-20'
WHERE   id = '[5,5]'
AND valid_at @> '2018-01-05'::timestamp;
ROLLBACK;


no_action_vs_restriction test2
BEGIN;
ALTER TABLE temporal_fk_rng2rng
DROP CONSTRAINT IF EXISTS temporal_fk_rng2rng_fk;
ALTER TABLE temporal_fk_rng2rng
ADD CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng
ON DELETE no action
ON UPDATE no action;
INSERT INTO temporal_rng VALUES ('[5,5]', tsrange('2018-01-01', '2018-02-01')),
 ('[5,5]', tsrange('2018-02-01', '2018-03-01'));
INSERT INTO temporal_fk_rng2rng VALUES ('[3,3]', tsrange('2018-01-05',
'2018-01-10'), '[5,5]');
/*
expect below command fail.
since to be deleted range is being referenced (in temporal_fk_rng2rng)
*/
UPDATE temporal_rng FOR PORTION OF valid_at FROM '2018-01-06' TO '2018-01-08'
SET id = '[7,7]'
WHERE   id = '[5,5]'
AND valid_at @> '2018-01-05'::timestamp;
TABLE temporal_rng \; table temporal_fk_rng2rng;

ROLLBACK;


no_action_vs_restriction test3
BEGIN;
ALTER TABLE temporal_fk_rng2rng
DROP CONSTRAINT IF EXISTS temporal_fk_rng2rng_fk;
ALTER TABLE temporal_fk_rng2rng
ADD CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng
ON DELETE RESTRICT
ON UPDATE RESTRICT;
INSERT INTO temporal_rng VALUES ('[5,5]', tsrange('2018-01-01', '2018-02-01')),
 ('[5,5]', tsrange('2018-02-01', '2018-03-01'));
INSERT INTO temporal_fk_rng2rng VALUES ('[3,3]', tsrange('2018-01-05',
'2018-01-10'), '[5,5]');

/*
expect the below command not to fail.
since to be deleted range is not being referenced in temporal_fk_rng2rng)
but the v13 patch will fail.
*/
UPDATE temporal_rng FOR PORTION OF valid_at FROM '2018-01-12' TO '2018-01-20'
SET id = '[7,7]'
WHERE   id = '[5,5]'
AND valid_at @> '2018-01-05'::timestamp;
ROLLBACK;

Re: SQL:2011 application time

[Paul A Jungwirth]

On Fri, Sep 8, 2023 at 2:35 AM jian he  wrote:
>
> hi.
> the following script makes the server crash (Segmentation fault).
> [snip]
>
> ALTER TABLE temporal_fk_rng2rng
> ADD CONSTRAINT temporal_fk_rng2rng_fk
> FOREIGN KEY (parent_id, PERIOD valid_at)
> REFERENCES temporal_rng
> on update set DEFAULT
> on delete set DEFAULT;

Thank you for the report! It looks like I forgot to handle implicit
column names after REFERENCES. The PERIOD part needs to get looked up
from the PK as we do for normal FK attrs. I'll add that to the next
patch.

Yours,
Paul

Re: SQL:2011 application time

[jian he]

hi.
the following script makes the server crash (Segmentation fault).

create schema test;
set search_path to test;
DROP TABLE  IF EXISTS temporal_rng;
CREATE TABLE temporal_rng (id int4range, valid_at daterange);
ALTER TABLE temporal_rng
ADD CONSTRAINT temporal_rng_pk
PRIMARY KEY (id, valid_at WITHOUT OVERLAPS);

INSERT INTO temporal_rng VALUES
('[11,11]', daterange('2018-01-01', '2020-01-01')),
('[11,11]', daterange('2020-01-01', '2021-01-01')),
('[20,20]', daterange('2018-01-01', '2020-01-01')),
('[20,20]', daterange('2020-01-01', '2021-01-01'));

DROP TABLE  IF EXISTS temporal_fk_rng2rng;

CREATE TABLE temporal_fk_rng2rng (
id int4range,
valid_at tsrange,
parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)
);
---
BEGIN;
ALTER TABLE temporal_fk_rng2rng DROP CONSTRAINT IF EXISTS
temporal_fk_rng2rng_fk;
ALTER TABLE temporal_fk_rng2rng
  ALTER COLUMN parent_id SET DEFAULT '[-1,-1]',
  ALTER COLUMN valid_at SET DEFAULT tsrange('2018-01-01', '2019-11-11');

ALTER TABLE temporal_fk_rng2rng
ADD CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal_rng
on update set DEFAULT
on delete set DEFAULT;
-gdb related
info:---
(gdb) continue
Continuing.

Program received signal SIGSEGV, Segmentation fault.
FindFKComparisonOperators (fkconstraint=0x556450100bd8,
tab=0x55644ff8f570, i=1, fkattnum=0x7ffeb3286ba0,
old_check_ok=0x7ffeb3286b11, old_pfeqop_item=0x7ffeb3286b28,
pktype=3912, fktype=3908, opclass=10078, is_temporal=true,
for_overlaps=true, pfeqopOut=0x7ffeb3286da4, ppeqopOut=0x7ffeb3286e24,
ffeqopOut=0x7ffeb3286ea4) at
../../Desktop/pg_sources/main/postgres/src/backend/commands/tablecmds.c:11582
11582   pkattr_name = strVal(fkconstraint->pk_period);
(gdb) where
#0  FindFKComparisonOperators (fkconstraint=0x556450100bd8,
tab=0x55644ff8f570, i=1,
fkattnum=0x7ffeb3286ba0, old_check_ok=0x7ffeb3286b11,
old_pfeqop_item=0x7ffeb3286b28, pktype=3912,
fktype=3908, opclass=10078, is_temporal=true, for_overlaps=true,
pfeqopOut=0x7ffeb3286da4,
ppeqopOut=0x7ffeb3286e24, ffeqopOut=0x7ffeb3286ea4)
at 
../../Desktop/pg_sources/main/postgres/src/backend/commands/tablecmds.c:11582
#1  0x55644e53875a in ATAddForeignKeyConstraint
(wqueue=0x7ffeb3287118, tab=0x55644ff8f570,
rel=0x7fb2dc124430, fkconstraint=0x556450100bd8, recurse=true,
recursing=false, lockmode=6)
at 
../../Desktop/pg_sources/main/postgres/src/backend/commands/tablecmds.c:10395
#2  0x55644e536cc2 in ATExecAddConstraint (wqueue=0x7ffeb3287118,
tab=0x55644ff8f570,
rel=0x7fb2dc124430, newConstraint=0x556450100bd8, recurse=true,
is_readd=false, lockmode=6)
at 
../../Desktop/pg_sources/main/postgres/src/backend/commands/tablecmds.c:9948
#3  0x55644e528eaa in ATExecCmd (wqueue=0x7ffeb3287118,
tab=0x55644ff8f570, cmd=0x5564500fae48,
lockmode=6, cur_pass=10, context=0x7ffeb3287310)
at 
../../Desktop/pg_sources/main/postgres/src/backend/commands/tablecmds.c:5711
#4  0x55644e5283f6 in ATRewriteCatalogs (wqueue=0x7ffeb3287118,
lockmode=6, context=0x7ffeb3287310)
at 
../../Desktop/pg_sources/main/postgres/src/backend/commands/tablecmds.c:5569
#5  0x55644e527031 in ATController (parsetree=0x55645000e228,
rel=0x7fb2dc124430,
cmds=0x55645000e1d8, recurse=true, lockmode=6, context=0x7ffeb3287310)
at 
../../Desktop/pg_sources/main/postgres/src/backend/commands/tablecmds.c:5136
#6  0x55644e526a9d in AlterTable (stmt=0x55645000e228, lockmode=6,
context=0x7ffeb3287310)
at 
../../Desktop/pg_sources/main/postgres/src/backend/commands/tablecmds.c:4789
#7  0x55644e92eb65 in ProcessUtilitySlow (pstate=0x55644ff8f460,
pstmt=0x55645000e2d8,
--Type  for more, q to quit, c to continue without paging--
55645000d330 "ALTER TABLE temporal_fk_rng2rng\n\tADD CONSTRAINT
temporal_fk_rng2rng_fk\n\tFOREIGN KEY (parent_id, PERIOD
valid_at)\n\tREFERENCES temporal_rng\non update set DEFAULT \n
   on delete set DEFAULT;", context=PROCESS_UTILITY_TOPLEVEL,
params=0x0, queryEnv=0x0,
dest=0x55645000e698, qc=0x7ffeb3287970)
at ../../Desktop/pg_sources/main/postgres/src/backend/tcop/utility.c:1329
#8  0x55644e92e24c in standard_ProcessUtility (pstmt=0x55645000e2d8,
queryString=0x55645000d330 "ALTER TABLE temporal_fk_rng2rng\n\tADD
CONSTRAINT temporal_fk_rng2rng_fk\n\tFOREIGN KEY (parent_id, PERIOD
valid_at)\n\tREFERENCES temporal_rng\non update set DEFAULT \n
   on delete set DEFAULT;", readOnlyTree=false,
context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
queryEnv=0x0, dest=0x55645000e698, qc=0x7ffeb3287970)
at ../../Desktop/pg_sources/main/postgres/src/backend/tcop/utility.c:1078
#9  0x55644e92c921 in ProcessUtility (pstmt=0x55645000e2d8,
queryString=0x55645000d330 "ALTER TABLE temporal_fk_rng2rng\n\tADD

Re: SQL:2011 application time

[jian he]

On Sat, Sep 2, 2023 at 5:58 AM Paul Jungwirth
 wrote:
>
>
> I don't quite understand this part:
>
>  >> Also, your implementation
>  >> requires at least one non-overlaps column, which also seems like a
>  >> confusing restriction.
>
> That's just a regular non-temporal constraint. Right? If I'm missing
> something let me know.
>

for a range primary key, is it fine to expect it to be unique, not
null and also not overlap? (i am not sure how hard to implement it).

-
quote from 7IWD2-02-Foundation-2011-12.pdf. 4.18.3.2 Unique
constraints, page 97 of 1483.

4.18.3.2 Unique constraints In addition to the components of every
table constraint descriptor, a unique constraint descriptor includes:
— An indication of whether it was defined with PRIMARY KEY or UNIQUE.
— The names and positions of the unique columns specified in the

 — If  is specified, then the name of
the period specified.

If the table descriptor for base table T includes a unique constraint
descriptor indicating that the unique constraint was defined with
PRIMARY KEY, then the columns of that unique constraint constitute the
primary key of T. A table that has a primary key cannot have a proper
supertable.
A unique constraint that does not include a  on a table T is satisfied if and only if there do not
exist two rows R1 and R2 of T such that R1 and R2 have the same
non-null values in the unique columns. If a unique constraint UC on a
table T includes a  WOS, then let
 ATPN be the contained in WOS. UC is
satisfied if and only if there do not exist two rows R1 and R2 of T
such that R1 and R2 have the same non-null values in the unique
columns and the ATPN period values of R1 and R2 overlap. In addition,
if the unique constraint was defined with PRIMARY KEY, then it
requires that none of the values in the specified column or columns be
a null value.
-
based on the above, the unique constraint does not specify that the
column list must be range type. UNIQUE (a, c WITHOUT OVERLAPS).
Here column "a" can be a range type (that have overlap property) and
can be not.
In fact, many of your primary key, foreign key regess test using
something like '[11,11]' (which make it more easy to understand),
which in logic is a non-range usage.
So UNIQUE (a, c WITHOUT OVERLAPS), column "a" be a non-range data type
does make sense?

Re: SQL:2011 application time

[Vik Fearing]

On 9/1/23 21:56, Paul Jungwirth wrote:

On 9/1/23 03:50, Vik Fearing wrote:

On 9/1/23 11:30, Peter Eisentraut wrote:
1) If I write UNIQUE (a, b, c WITHOUT OVERLAPS), does the WITHOUT 
OVERLAPS clause attach to the last column, or to the whole column 
list? In the SQL standard, you can only have one period and it has to 
be listed last, so this question does not arise.  But here we are 
building a more general facility to then build the SQL facility on 
top of.  So I think it doesn't make sense that the range column must 
be last or that there can only be one.  Also, your implementation 
requires at least one non-overlaps column, which also seems like a 
confusing restriction.


I think the WITHOUT OVERLAPS clause should be per-column, so that 
something like UNIQUE (a WITHOUT OVERLAPS, b, c WITHOUT OVERLAPS) 
would be possible.  Then the WITHOUT OVERLAPS clause would directly 
correspond to the choice between equality or overlaps operator per 
column.


An alternative interpretation would be that WITHOUT OVERLAPS applies 
to the whole column list, and we would take it to mean, for any range 
column, use the overlaps operator, for any non-range column, use the 
equals operator.  But I think this would be confusing and would 
prevent the case of using the equality operator for some ranges and 
the overlaps operator for some other ranges in the same key.


I prefer the first option.  That is: WITHOUT OVERLAPS applies only to 
the column or expression it is attached to, and need not be last in line.


I agree. The second option seems confusing and is more restrictive.

I think allowing multiple uses of `WITHOUT OVERLAPS` (and in any 
position) is a great recommendation that enables a lot of new 
functionality. Several books[1,2] about temporal databases describe a 
multi-dimensional temporal space (even beyond application time vs. 
system time), and the standard is pretty disappointing here. It's not a 
weird idea.


But I just want to be explicit that this isn't something the standard 
describes. (I think everyone in the conversation so far understands 
that.) So far I've tried to be pretty scrupulous about following 
SQL:2011, although personally I'd rather see Postgres support this 
functionality. And it's not like it goes *against* what the standard 
says. But if there are any objections, I'd love to hear them before 
putting in the work. :-)



I have no problem with a first version doing exactly what the standard 
says and expanding it later.



If we allow multiple+anywhere WITHOUT OVERLAPS in PRIMARY KEY & UNIQUE 
constraints, then surely we also allow multiple+anywhere PERIOD in 
FOREIGN KEY constraints too. (I guess the standard switched keywords 
because a FK is more like "MUST OVERLAPS". :-)



Seems reasonable.


Also if you have multiple application-time dimensions we probably need 
to allow multiple FOR PORTION OF clauses. I think the syntax would be:


UPDATE t
   FOR PORTION OF valid_at FROM ... TO ...
   FOR PORTION OF asserted_at FROM ... TO ...
   [...]
   SET foo = bar

Does that sound okay?



That sounds really cool.


[1] C. J. Date, Hugh Darwen, Nikos Lorentzos. Time and Relational 
Theory, Second Edition: Temporal Databases in the Relational Model and 
SQL. 2nd edition, 2014.

[2] Tom Johnston. Bitemporal Data: Theory and Practice. 2014.



Thanks!  I have ordered these books.
--
Vik Fearing

Re: SQL:2011 application time

[Paul Jungwirth]

On 9/1/23 03:50, Vik Fearing wrote:

On 9/1/23 11:30, Peter Eisentraut wrote:
1) If I write UNIQUE (a, b, c WITHOUT OVERLAPS), does the WITHOUT 
OVERLAPS clause attach to the last column, or to the whole column 
list? In the SQL standard, you can only have one period and it has to 
be listed last, so this question does not arise.  But here we are 
building a more general facility to then build the SQL facility on top 
of.  So I think it doesn't make sense that the range column must be 
last or that there can only be one.  Also, your implementation 
requires at least one non-overlaps column, which also seems like a 
confusing restriction.


I think the WITHOUT OVERLAPS clause should be per-column, so that 
something like UNIQUE (a WITHOUT OVERLAPS, b, c WITHOUT OVERLAPS) 
would be possible.  Then the WITHOUT OVERLAPS clause would directly 
correspond to the choice between equality or overlaps operator per 
column.


An alternative interpretation would be that WITHOUT OVERLAPS applies 
to the whole column list, and we would take it to mean, for any range 
column, use the overlaps operator, for any non-range column, use the 
equals operator.  But I think this would be confusing and would 
prevent the case of using the equality operator for some ranges and 
the overlaps operator for some other ranges in the same key.


I prefer the first option.  That is: WITHOUT OVERLAPS applies only to 
the column or expression it is attached to, and need not be last in line.


I agree. The second option seems confusing and is more restrictive.

I think allowing multiple uses of `WITHOUT OVERLAPS` (and in any 
position) is a great recommendation that enables a lot of new 
functionality. Several books[1,2] about temporal databases describe a 
multi-dimensional temporal space (even beyond application time vs. 
system time), and the standard is pretty disappointing here. It's not a 
weird idea.


But I just want to be explicit that this isn't something the standard 
describes. (I think everyone in the conversation so far understands 
that.) So far I've tried to be pretty scrupulous about following 
SQL:2011, although personally I'd rather see Postgres support this 
functionality. And it's not like it goes *against* what the standard 
says. But if there are any objections, I'd love to hear them before 
putting in the work. :-)


If we allow multiple+anywhere WITHOUT OVERLAPS in PRIMARY KEY & UNIQUE 
constraints, then surely we also allow multiple+anywhere PERIOD in 
FOREIGN KEY constraints too. (I guess the standard switched keywords 
because a FK is more like "MUST OVERLAPS". :-)


Also if you have multiple application-time dimensions we probably need 
to allow multiple FOR PORTION OF clauses. I think the syntax would be:


UPDATE t
  FOR PORTION OF valid_at FROM ... TO ...
  FOR PORTION OF asserted_at FROM ... TO ...
  [...]
  SET foo = bar

Does that sound okay?

I don't quite understand this part:

>> Also, your implementation
>> requires at least one non-overlaps column, which also seems like a
>> confusing restriction.

That's just a regular non-temporal constraint. Right? If I'm missing 
something let me know.


[1] C. J. Date, Hugh Darwen, Nikos Lorentzos. Time and Relational 
Theory, Second Edition: Temporal Databases in the Relational Model and 
SQL. 2nd edition, 2014.

[2] Tom Johnston. Bitemporal Data: Theory and Practice. 2014.

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

Re: SQL:2011 application time

[Vik Fearing]

On 9/1/23 11:30, Peter Eisentraut wrote:
1) If I write UNIQUE (a, b, c WITHOUT OVERLAPS), does the WITHOUT 
OVERLAPS clause attach to the last column, or to the whole column list? 
In the SQL standard, you can only have one period and it has to be 
listed last, so this question does not arise.  But here we are building 
a more general facility to then build the SQL facility on top of.  So I 
think it doesn't make sense that the range column must be last or that 
there can only be one.  Also, your implementation requires at least one 
non-overlaps column, which also seems like a confusing restriction.


I think the WITHOUT OVERLAPS clause should be per-column, so that 
something like UNIQUE (a WITHOUT OVERLAPS, b, c WITHOUT OVERLAPS) would 
be possible.  Then the WITHOUT OVERLAPS clause would directly correspond 
to the choice between equality or overlaps operator per column.


An alternative interpretation would be that WITHOUT OVERLAPS applies to 
the whole column list, and we would take it to mean, for any range 
column, use the overlaps operator, for any non-range column, use the 
equals operator.  But I think this would be confusing and would prevent 
the case of using the equality operator for some ranges and the overlaps 
operator for some other ranges in the same key.


I prefer the first option.  That is: WITHOUT OVERLAPS applies only to 
the column or expression it is attached to, and need not be last in line.

--
Vik Fearing

Re: SQL:2011 application time

[Peter Eisentraut]

On 31.08.23 23:26, Paul Jungwirth wrote:
I've tried to clean up the first four patches to get them ready for 
committing, since they could get committed before the PERIOD patch. I 
think there is a little more cleanup needed but they should be ready for 
a review.


Looking at the patch 0001 "Add temporal PRIMARY KEY and UNIQUE constraints":

Generally, this looks like a good direction.  The patch looks 
comprehensive, with documentation and tests, and appears to cover all 
the required pieces (client programs, ruleutils, etc.).



I have two conceptual questions that should be clarified before we go 
much further:


1) If I write UNIQUE (a, b, c WITHOUT OVERLAPS), does the WITHOUT 
OVERLAPS clause attach to the last column, or to the whole column list? 
In the SQL standard, you can only have one period and it has to be 
listed last, so this question does not arise.  But here we are building 
a more general facility to then build the SQL facility on top of.  So I 
think it doesn't make sense that the range column must be last or that 
there can only be one.  Also, your implementation requires at least one 
non-overlaps column, which also seems like a confusing restriction.


I think the WITHOUT OVERLAPS clause should be per-column, so that 
something like UNIQUE (a WITHOUT OVERLAPS, b, c WITHOUT OVERLAPS) would 
be possible.  Then the WITHOUT OVERLAPS clause would directly correspond 
to the choice between equality or overlaps operator per column.


An alternative interpretation would be that WITHOUT OVERLAPS applies to 
the whole column list, and we would take it to mean, for any range 
column, use the overlaps operator, for any non-range column, use the 
equals operator.  But I think this would be confusing and would prevent 
the case of using the equality operator for some ranges and the overlaps 
operator for some other ranges in the same key.


2) The logic hinges on get_index_attr_temporal_operator(), to pick the 
equality and overlaps operator for each column.  For btree indexes, the 
strategy numbers are fixed, so this is straightforward.  But for gist 
indexes, the strategy numbers are more like recommendations.  Are we 
comfortable with how this works?  I mean, we could say, if you want to 
be able to take advantage of the WITHOUT OVERLAPS syntax, you have to 
use these numbers, otherwise you're on your own.  It looks like the gist 
strategy numbers are already hardcoded in a number of places, so maybe 
that's all okay, but I feel we should be more explicit about this 
somewhere, maybe in the documentation, or at least in code comments.



Besides that, some stylistic comments:

* 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".


* 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-e6a19d33c...@eisentraut.org


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


* 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.)

Re: SQL:2011 application time

[Corey Huinker]

>
> The PERIOD patch is not finished and includes some deliberately-failing
> tests. I did make some progress here finishing ALTER TABLE ADD PERIOD.
>

If it's ok with you, I need PERIODs for System Versioning, and planned on
developing a highly similar version, albeit closer to the standard. It
shouldn't interfere with your work as you're heavily leveraging range
types.

Re: SQL:2011 application time

[jian he]

On Fri, Jul 7, 2023 at 9:04 AM Paul A Jungwirth
 wrote:
>
> On Thu, Jul 6, 2023 at 1:13 AM Peter Eisentraut
>  wrote:
> >
> > I had talked to Paul about this offline a while ago.  btree_gist to core
> > is no longer considered a prerequisite.  But Paul was planning to
> > produce a new patch set that is arranged and sequenced a bit
> > differently.  Apparently, that new version is not done yet, so it would
> > make sense to either close this entry as returned with feedback, or move
> > it to the next commit fest as waiting on author.
>
> Here are some new patch files based on discussions from PGCon. The
> patches are reorganized a bit to hopefully make them easier to review:
>
> Initially I implement all functionality on just range columns, without
> supporting PERIODs yet. There are patches for temporal PRIMARY
> KEY/UNIQUE constraints, for simple foreign keys (without CASCADE/SET
> NULL/SET DEFAULT), for UPDATE/DELETE FOR PORTION OF, and then for the
> rest of the FK support (which depends on FOR PORTION OF). If you
> compare these patches to the v11 ones, you'll see that a ton of
> clutter disappears by not supporting PERIODs as a separate "thing".
>
> Finally there is a patch adding PERIOD syntax, but with a new
> implementation where a PERIOD causes us to just define a GENERATED
> range column. That means we can support all the same things as before
> but without adding the clutter. This patch isn't quite working yet
> (especially ALTER TABLE), but I thought I'd send where I'm at so far,
> since it sounds like folks are interested in doing a review. Also it
> was a little tricky dealing with the dependency between the PERIOD and
> the GENERATED column. (See the comments in the patch.) If anyone has a
> suggestion there I'd be happy to hear it.
>
> My goal is to include another patch soon to support hidden columns, so
> that the period's GENERATED column can be hidden. I read the
> conversation about a recent patch attempt for something similar, and I
> think I can use most of that (but cut some of the things the community
> was worried about).
>
> All these patches need some polishing, but I think there is enough new
> here for them to be worth reading for anyone interested in temporal
> progress.
>
> I'll set this commitfest entry back to Needs Review. Thanks for taking a look!
>
> Paul

due to change in:
https://www.postgresql.org/message-id/flat/ec8b1d9b-502e-d1f8-e909-1bf9dffe6...@illuminatedcomputing.com

git apply 
$DOWNLOADS/patches/v12-0001-Add-temporal-PRIMARY-KEY-and-UNIQUE-constraints.patch
error: patch failed: src/backend/commands/indexcmds.c:940
error: src/backend/commands/indexcmds.c: patch does not apply

probably need some adjustment.

Re: SQL:2011 application time

[Peter Eisentraut]

On 07.07.23 03:03, Paul A Jungwirth wrote:

Here are some new patch files based on discussions from PGCon.


Here are a few fixup patches to get things building without warnings and 
errors.


The last patch (your 0005) fails the regression test for me and it 
didn't appear to be a trivial problem, so please take another look at 
that sometime.  (Since it's the last patch, it's obviously lower priority.)
From 6694912b0fdd8742e583ff2a524f32284c330711 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 12 Jul 2023 09:45:56 +0200
Subject: [PATCH v12] fixup! Add temporal PRIMARY KEY and UNIQUE constraints

---
 src/bin/pg_dump/pg_dump.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a95e1d3696..33ad34ad66 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7029,16 +7029,14 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int 
numTables)
 "(SELECT 
pg_catalog.array_agg(attstattarget ORDER BY attnum) "
 "  FROM 
pg_catalog.pg_attribute "
 "  WHERE attrelid = 
i.indexrelid AND "
-"attstattarget >= 
0) AS indstatvals, "
-"c.conexclop IS NOT 
NULL AS withoutoverlaps, ");
+"attstattarget >= 
0) AS indstatvals, ");
else
appendPQExpBufferStr(query,
 "0 AS parentidx, "
 "i.indnatts AS 
indnkeyatts, "
 "i.indnatts AS 
indnatts, "
 "'' AS indstatcols, "
-"'' AS indstatvals, "
-"null AS 
withoutoverlaps, ");
+"'' AS indstatvals, ");
 
if (fout->remoteVersion >= 15)
appendPQExpBufferStr(query,
@@ -7047,6 +7045,13 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int 
numTables)
appendPQExpBufferStr(query,
 "false AS 
indnullsnotdistinct, ");
 
+   if (fout->remoteVersion >= 17)
+   appendPQExpBufferStr(query,
+"c.conexclop IS NOT 
NULL AS withoutoverlaps ");
+   else
+   appendPQExpBufferStr(query,
+"null AS 
withoutoverlaps ");
+
/*
 * The point of the messy-looking outer join is to find a constraint 
that
 * is related by an internal dependency link to the index. If we find 
one,
-- 
2.41.0

From 49932adb8e626036b8d8edf26ed1465f39db82bd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 12 Jul 2023 10:16:53 +0200
Subject: [PATCH v12] fixup! Add UPDATE/DELETE FOR PORTION OF

---
 doc/src/sgml/ref/delete.sgml | 2 ++
 doc/src/sgml/ref/update.sgml | 2 ++
 src/backend/parser/analyze.c | 5 +++--
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/ref/delete.sgml b/doc/src/sgml/ref/delete.sgml
index 868cf0d1f9..aec593239b 100644
--- a/doc/src/sgml/ref/delete.sgml
+++ b/doc/src/sgml/ref/delete.sgml
@@ -55,6 +55,7 @@ Description
circumstances.
   
 
+  
 
   
The optional RETURNING clause causes 
DELETE
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index f2042e0b25..62e9e0e1f0 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -52,6 +52,7 @@ Description
circumstances.
   
 
+  
 
   
The optional RETURNING clause causes 
UPDATE
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 0b2109d1bb..c6d2b7e1d1 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1247,6 +1247,7 @@ transformForPortionOfClause(ParseState *pstate,
char *range_name = forPortionOf->range_name;
char *range_type_name = NULL;
int range_attno = InvalidAttrNumber;
+   Form_pg_attribute attr;
ForPortionOfExpr *result;
List *targetList;
Node *target_start, *target_end;
@@ -1264,7 +1265,7 @@ transformForPortionOfClause(ParseState *pstate,
range_name,

RelationGetRelationName(targetrel)),
 parser_errposition(pstate, 
forPortionOf->range_name_location)));
-   Form_pg_attribute attr = TupleDescAttr(targetrel->rd_att, range_attno - 
1);
+   attr = TupleDescAttr(targetrel->rd_att, range_attno 

Re: SQL:2011 application time

[Daniel Gustafsson]

> On 6 Jul 2023, at 10:12, Peter Eisentraut  
> wrote:

> it would make sense to either close this entry as returned with feedback, or 
> move it to the next commit fest as waiting on author.

Fair enough, done.

--
Daniel Gustafsson

Re: SQL:2011 application time

[Peter Eisentraut]

On 04.07.23 14:48, Daniel Gustafsson wrote:

On 8 May 2023, at 09:10, Peter Eisentraut  
wrote:

On 03.05.23 23:02, Paul Jungwirth wrote:

Thank you again for the review. Here is a patch with most of your feedback 
addressed. Sorry it has taken so long! These patches are rebased up to 
1ab763fc22adc88e5d779817e7b42b25a9dd7c9e
(May 3).


Here are a few small fixup patches to get your patch set compiling cleanly.

Also, it looks like the patches 0002, 0003, and 0004 are not split up 
correctly.  0002 contains tests using the FOR PORTION OF syntax introduced in 
0003, and 0003 uses the function build_period_range() from 0004.


These patches no longer apply without a new rebase.  Should this patch be
closed in while waiting for the prequisite of adding btree_gist to core
mentioned upthread?  I see no patch registered in the CF for this unless I'm
missing sometihng.


I had talked to Paul about this offline a while ago.  btree_gist to core 
is no longer considered a prerequisite.  But Paul was planning to 
produce a new patch set that is arranged and sequenced a bit 
differently.  Apparently, that new version is not done yet, so it would 
make sense to either close this entry as returned with feedback, or move 
it to the next commit fest as waiting on author.

Re: SQL:2011 application time

[Daniel Gustafsson]

> On 8 May 2023, at 09:10, Peter Eisentraut  
> wrote:
> 
> On 03.05.23 23:02, Paul Jungwirth wrote:
>> Thank you again for the review. Here is a patch with most of your feedback 
>> addressed. Sorry it has taken so long! These patches are rebased up to 
>> 1ab763fc22adc88e5d779817e7b42b25a9dd7c9e
>> (May 3).
> 
> Here are a few small fixup patches to get your patch set compiling cleanly.
> 
> Also, it looks like the patches 0002, 0003, and 0004 are not split up 
> correctly.  0002 contains tests using the FOR PORTION OF syntax introduced in 
> 0003, and 0003 uses the function build_period_range() from 0004.

These patches no longer apply without a new rebase.  Should this patch be
closed in while waiting for the prequisite of adding btree_gist to core
mentioned upthread?  I see no patch registered in the CF for this unless I'm
missing sometihng.

--
Daniel Gustafsson

Re: SQL:2011 application time

[Peter Eisentraut]

On 03.05.23 23:02, Paul Jungwirth wrote:
Thank you again for the review. Here is a patch with most of your 
feedback addressed. Sorry it has taken so long! These patches are 
rebased up to 1ab763fc22adc88e5d779817e7b42b25a9dd7c9e

(May 3).


Here are a few small fixup patches to get your patch set compiling cleanly.

Also, it looks like the patches 0002, 0003, and 0004 are not split up 
correctly.  0002 contains tests using the FOR PORTION OF syntax 
introduced in 0003, and 0003 uses the function build_period_range() from 
0004.
From 6fa819b675255af763e51a67d4d8c88f1d390b6c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 8 May 2023 08:45:32 +0200
Subject: [PATCH 1/3] fixup! Add PERIODs

---
 doc/src/sgml/ref/alter_table.sgml| 2 +-
 src/test/modules/test_ddl_deparse/test_ddl_deparse.c | 6 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml 
b/doc/src/sgml/ref/alter_table.sgml
index 487f09f88a..d6aed3dff8 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -582,7 +582,7 @@ Description
 

 
-   
+   
 DROP PERIOD FOR
 
  
diff --git a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c 
b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
index b7c6f98577..6f4e44de3f 100644
--- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
+++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
@@ -309,6 +309,12 @@ get_altertable_subcmdinfo(PG_FUNCTION_ARGS)
case AT_ReAddStatistics:
strtype = "(re) ADD STATS";
break;
+   case AT_AddPeriod:
+   strtype = "ADD PERIOD";
+   break;
+   case AT_DropPeriod:
+   strtype = "DROP PERIOD";
+   break;
}
 
if (subcmd->recurse)
-- 
2.40.0

From 809e1fe145896b190aa4c0ec73902071e5ccdccc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 8 May 2023 09:04:42 +0200
Subject: [PATCH 2/3] fixup! Add PERIODs

---
 src/backend/catalog/meson.build | 1 +
 src/include/catalog/meson.build | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/backend/catalog/meson.build b/src/backend/catalog/meson.build
index fa6609e577..c499cd2f5d 100644
--- a/src/backend/catalog/meson.build
+++ b/src/backend/catalog/meson.build
@@ -26,6 +26,7 @@ backend_sources += files(
   'pg_namespace.c',
   'pg_operator.c',
   'pg_parameter_acl.c',
+  'pg_period.c',
   'pg_proc.c',
   'pg_publication.c',
   'pg_range.c',
diff --git a/src/include/catalog/meson.build b/src/include/catalog/meson.build
index 3179be09d3..c92d4928a3 100644
--- a/src/include/catalog/meson.build
+++ b/src/include/catalog/meson.build
@@ -57,6 +57,7 @@ catalog_headers = [
   'pg_collation.h',
   'pg_parameter_acl.h',
   'pg_partitioned_table.h',
+  'pg_period.h',
   'pg_range.h',
   'pg_transform.h',
   'pg_sequence.h',
-- 
2.40.0

From 94f46deacdeaa3dbac1d3988678981ac8cf5fa9a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 8 May 2023 09:05:04 +0200
Subject: [PATCH 3/3] fixup! Add UPDATE/DELETE FOR PORTION OF

---
 src/backend/utils/adt/meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/backend/utils/adt/meson.build 
b/src/backend/utils/adt/meson.build
index 8515cd9365..9deb26f953 100644
--- a/src/backend/utils/adt/meson.build
+++ b/src/backend/utils/adt/meson.build
@@ -65,6 +65,7 @@ backend_sources += files(
   'oracle_compat.c',
   'orderedsetaggs.c',
   'partitionfuncs.c',
+  'period.c',
   'pg_locale.c',
   'pg_lsn.c',
   'pg_upgrade_support.c',
-- 
2.40.0