On Fri, Aug 2, 2019 at 1:49 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote: > I did some clean-up on this patch. I have also refactored a small portion of the code > to reduce the footprint of the patch. For simplicity, I have divided the patch into 6 > patches, now it is easy to review and debug. > Please follow the PostgreSQL coding guidelines. I have found places where you missed that, secondly code even in WIP stage must not have WARNING because it looks ugly.
Thank you for the cleanup Ibrar! I'll try to stick to the coding standards more closely going forward. If you have any review comments I would certainly appreciate them, especially about the overall approach. I know that the implementation in its current form is not very tasteful, but I wanted to get some feedback on the ideas.
I have reviewed the main design, and in my opinion, it is a good start.
- Why we are not allowing any other datatype other than ranges in the
primary key. Without that there is no purpose of a primary key.
- Thinking about some special token to differentiate between normal
primary key and temporal primary key
Also just to reiterate: this patch depends on my other CF entry (range_agg), whose scope has expanded considerably. Right now I'm focusing on that. And if you're trying to make this code work, it's important to apply the range_agg patch first, since the temporal foreign key implementation calls that function.
Also: since this patch raises the question of how to conform to SQL:2011 while still supporting Postgres range types, I wrote an article that surveys SQL:2011 temporal features in MariaDB, DB2, Oracle, and MS SQL Server:
- Everyone lets you define PERIODs, but what you can do with them is still *very* limited. - No one treats PERIODs as first-class types or expressions; they are more like table metadata.
- Oracle PERIODs do permit NULL start/end values, and it interprets them as "unbounded". That goes against the standard but since it's what Postgres does with ranges, it suggests to me that maybe we should follow their lead. Anyway I think a NULL is nicer than a sentinel for this purpose.
That is an open debate, that we want to strictly follow the standard or map that
to PostgreSQL range type which allows NULL. But how you will define a primary