On 05/01/2020 11:16, Surafel Temesgen wrote:
>
>
> On Fri, Jan 3, 2020 at 4:22 PM Vik Fearing
> <vik.fearing@2ndquadrant.com <mailto:vik.fearing@2ndquadrant.com>> wrote:
>
> >
> > Rebased and conflict resolved i hope it build clean this time
> >
>
> It does but you haven't included your tests file so `make check`
> fails.
>
>
>
> what tests file?
Exactly.
> i add system_versioned_table.sql and system_versioned_table.out
> test files
Those are not included in the patch.
<checks again>
Okay, that was user error on my side. I apologize.
>
>
> It seems clear to me that you haven't tested it at all anyway. The
> temporal conditions do not return the correct results, and the
> syntax is
> wrong, too. Also, none of my previous comments have been addressed
> except for "system versioning" instead of "system_versioning". Why?
>
>
> I also correct typo and add row end column time to unique
> key that make it unique for current data. As you mentioned
> other comment is concerning about application-time periods
> which the patch not addressing .
- For performance, you must put the start column in the indexes also.
- You only handle timestamp when you should also handle timestamptz and
date.
- You don't throw 2201H for anomalies
> i refer sql 2011 standard for
> syntax can you tell me which syntax you find it wrong?
Okay, now that I see your tests, I understand why everything is broken.
You only test FROM-TO and with a really wide interval. There are no
tests for AS OF and no tests for BETWEEN-AND.
As for the syntax, you have:
select a from for stest0 system_time from '2000-01-01 00:00:00.00000' to
'infinity' ORDER BY a;
when you should have:
select a from stest0 for system_time from '2000-01-01 00:00:00.00000' to
'infinity' ORDER BY a;
That is, the FOR should be on the other side of the table name.
In addition, there are many rules in the standard that are not respected
here. For example, this query works and should not:
CREATE TABLE t (system_time integer) WITH SYSTEM VERSIONING;
This syntax is not supported:
ALTER TABLE t
ADD PERIOD FOR SYSTEM_TIME (s, e)
ADD COLUMN s timestamp
ADD COLUMN e timestamp;
psql's \d does not show that the table is system versioned, and doesn't
show the columns of the system_time period.
I can drop columns used in the period.
Please don't hesitate to take inspiration from my extension that does
this. The extension is under the PostgreSQL license for that reason.
Take from it whatever you need.
https://github.com/xocolatl/periods/
--
Vik Fearing
