Horton

Re: SQL:2011 application time

Paul A Jungwirth <pj@illuminatedcomputing.com>

View thread
From: Paul Jungwirth <pj@illuminatedcomputing.com>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Corey Huinker <corey.huinker@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Jaime Casanova <jcasanov@systemguards.com.ec>
Date: 2023-05-03T21:02:47Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Rename gist stratnum support function

  2. Remove support for temporal RESTRICT foreign keys

  3. Cache NO ACTION foreign keys separately from RESTRICT foreign keys

  4. Fix NO ACTION temporal foreign keys when the referenced endpoints change

  5. Improve whitespace in without_overlaps test

  6. Tests for logical replication with temporal keys

  7. Support for GiST in get_equal_strategy_number()

  8. Make the conditions in IsIndexUsableForReplicaIdentityFull() more explicit

  9. Replace get_equal_strategy_number_for_am() by get_equal_strategy_number()

  10. Improve internal logical replication error for missing equality strategy

  11. Simplify IsIndexUsableForReplicaIdentityFull()

  12. Fix ALTER TABLE / REPLICA IDENTITY for temporal tables

  13. doc: Update pg_constraint.conexclop docs for WITHOUT OVERLAPS

  14. doc: Add PERIOD to ALTER TABLE reference docs

  15. doc: Add WITHOUT OVERLAPS to ALTER TABLE reference docs

  16. Add temporal FOREIGN KEY contraints

  17. Add temporal PRIMARY KEY and UNIQUE constraints

  18. Add stratnum GiST support function

  19. Avoid crashing when a JIT-inlined backend function throws an error.

  20. Revert temporal primary keys and foreign keys

  21. Fix ON CONFLICT DO NOTHING/UPDATE for temporal indexes

  22. Add test for REPLICA IDENTITY with a temporal key

  23. Use half-open interval notation in without_overlaps tests

  24. Use daterange and YMD in without_overlaps tests instead of tsrange.

  25. Rename pg_constraint.conwithoutoverlaps to conperiod

  26. Fix comment on gist_stratnum_btree

  27. Add missing TAP test name

  28. Improve error handling of HMAC computations

  29. Rename functions to avoid future conflicts

Attachments

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@illuminatedcomputing.com