Re: SQL:2011 application time

Hello,

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

The big change is switching from implementing FOR PORTION OF with 
triggers back to an executor node implementation. I think this is a lot 
simpler and means we don't have to be so "premeditated" (for example you 
just need a PERIOD/range, not a temporal PK).

I've also made some progress on partitioning temporal tables. It still 
needs some work though, and also it depends on my separate commitfest 
entry (https://commitfest.postgresql.org/43/4065/). So I've left it out 
of the patches attached here.

A few more details below:

Back in January 2022, Peter Eisentraut wrote:
> make_period_not_backward(): Hardcoding the name of the operator as "<"
> is not good.  You should perhaps lookup the less-than operator in the
> type cache.  Look around for TYPECACHE_LT_OPR for how this is usually done. 
> ...
> transformIndexConstraint(): As above, we can't look up the && operator
> by name.  In this case, I suppose we should look it up through the
> index AM support operators.

I've changed most locations to look up the operators we need using 
strategy number. But in some places I need the range intersects operator 
(`*`) and we don't have a strategy number for that. I don't really 
understand the purpose of not hardcoding operator names here. Can you 
give me the reasons for that? Do you have any suggestions what I can do 
to use `*`? Also, when I'm doing these operator lookups, do I need 
permission checks similar to what I see in ComputeIndexAttrs?

> Further, the additions to this function are very complicated and not
> fully explained.  I'm suspicious about things like
> findNewOrOldColumn() -- generally we should look up columns by number
> not name.  Perhaps you can add a header comment or split out the code
> further into smaller functions.

I still have some work to do on this. I agree it's very complicated, so 
I'm going to see what kind of refactoring I can do.

>>> I didn't follow why indexes would have periods, for example, the new
>>> period field in IndexStmt.  Is that explained anywhere?
>>
>> When you create a primary key or a unique constraint (which are backed
>> by a unique index), you can give a period name to make it a temporal
>> constraint. We create the index first and then create the constraint
>> as a side-effect of that (e.g. index_create calls
>> index_constraint_create). The analysis phase generates an IndexStmt.
>> So I think this was mostly a way to pass the period info down to the
>> constraint. It probably doesn't actually need to be stored on pg_index
>> though. Maybe it does for index_concurrently_create_copy. I'll add
>> some comments, but if you think it's the wrong approach let me know.
> 
> This seems backwards.  Currently, when you create a constraint, the index is created as a side effect and is owned,
soto speak, by the constraint.  What you are describing here sounds like the index owns the constraint.  This needs to
bereconsidered, I think.
 

After looking at this again I do think to reference the period from the 
index, not vice versa. The period is basically one of the index elements 
(e.g. `PRIMARY KEY (id, valid_at WITHOUT OVERLAPS)`). You can define a 
`PERIOD` without an index, but you can't define a WITHOUT OVERLAPS index 
without a period. In addition you could have multiple indexes using the 
same period (though this is probably unusual and technically disallowed 
by the standard, although in principal you could do it), but not 
multiple periods within the same index. I understand what you're saying 
about how constraints cause indexes as a by-product, but here the 
constraint isn't the PERIOD; it's the PRIMARY KEY or UNIQUE constraint. 
The PERIOD is just something the constraint & index refer to (like an 
expression indexElem). The dependency direction also suggests the period 
should be referenced by the index: you can drop the index without 
dropping the period, but dropping the period would cascade to dropping 
the index (or fail). I hope that makes sense. But let me know if you 
still disagree.

> Do we really need different trigger names depending on whether the
> foreign key is temporal? 

They don't have to be different. I used separate C functions because I 
didn't want standard FKs to be slowed/complicated by the temporal ones, 
and also I wanted to avoid merge conflicts with the work on avoiding SPI 
in RI checks. But you're just asking about the trigger names, right? I 
haven't changed those yet but it shouldn't take long.

> IMO, if this temporal feature is to happen, btree_gist needs to be moved 
> into core first.  Having to install an extension in order to use an 
> in-core feature like this isn't going to be an acceptable experience.

As far as I can tell the conversation about moving this into core hasn't 
gone anywhere. Do you still think this is a prerequisite to this patch? 
Is there anything I can do to help move `btree_gist` forward? It seems 
like a large backwards compatibility challenge. I imagine that getting 
agreement on how to approach it is actually more work than doing the 
development. I'd be very happy for any suggestions here!

Yours,

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