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, so to speak, by the constraint. What you are describing here sounds like the index owns the constraint. This needs to be reconsidered, 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(at)illuminatedcomputing(dot)com