public inbox for [email protected]  

help / color / mirror / Atom feed

From: Vik Fearing <[email protected]>
To: Surafel Temesgen <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: WIP: System Versioned Temporal Table
Date: Sun, 5 Jan 2020 13:50:36 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALAY4q8=ukp0xh5mS78RAxQxAUGpw_qn-_-OzsGMSvjC4hOYNA@mail.gmail.com>
References: <CALAY4q-cXCD0r4OybD=w7Hr7F026ZUY6=LMsVPUe6yw_PJpTKQ@mail.gmail.com>
	<[email protected]>
	<CALAY4q9u4r_J-=v_MXJTpEx6WA=YhT3A3n9sw6P2XxK0rWk5bg@mail.gmail.com>
	<[email protected]>
	<CALAY4q8xpuU7Qz7qy+5F8bh=GPAwR-pyscorAzTpyTQGyZbqtQ@mail.gmail.com>
	<[email protected]>
	<CALAY4q8ETRoFLBR8qz1P=SNskBBW5z-Kr90t_XGrm9mc9zCEbQ@mail.gmail.com>
	<[email protected]>
	<CALAY4q_voAROyjHL0fidvfY7=GGERjqSf0r=uGwrJ-T_9-VBKg@mail.gmail.com>
	<[email protected]>
	<CALAY4q88-1P1CWCJKEGHy1S3azG5g8cP_z_sEh+cd0G14YfmgA@mail.gmail.com>
	<[email protected]>
	<CALAY4q8=ukp0xh5mS78RAxQxAUGpw_qn-_-OzsGMSvjC4hOYNA@mail.gmail.com>

On 05/01/2020 11:16, Surafel Temesgen wrote:
>
>
> On Fri, Jan 3, 2020 at 4:22 PM Vik Fearing
> <[email protected] <mailto:[email protected]>> 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

view thread (97+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: WIP: System Versioned Temporal Table
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox