Re: WIP: System Versioned Temporal Table

On Fri, Jan 8, 2021 at 5:34 AM Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com>
wrote:

> On Fri, Jan 8, 2021 at 7:34 AM Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com>
> wrote:
> >
> > On Fri, Jan 8, 2021 at 7:13 AM Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com>
> wrote:
> >
> > > I've minimally rebased the patch to current head so that it compiles
> > > and passes current make check.
> >
> > Full version attached
>
> New version attached with improved error messages, some additional
> docs and a review of tests.
>
>
The v10 patch fails to make on the current master branch (15b824da). Error:

make[2]: Entering directory
'/var/lib/postgresql/git/postgresql/src/backend/parser'
'/usr/bin/perl' ./check_keywords.pl gram.y
../../../src/include/parser/kwlist.h
/usr/bin/bison -Wno-deprecated -d -o gram.c gram.y
gram.y:3685.55-56: error: $4 of ‘ColConstraintElem’ has no declared type
n->contype = ($4)->contype;
^^
gram.y:3687.56-57: error: $4 of ‘ColConstraintElem’ has no declared type
n->raw_expr = ($4)->raw_expr;
^^
gram.y:3734.41-42: error: $$ of ‘generated_type’ has no declared type
$$ = n;
^^
gram.y:3741.41-42: error: $$ of ‘generated_type’ has no declared type
$$ = n;
^^
gram.y:3748.41-42: error: $$ of ‘generated_type’ has no declared type
$$ = n;
^^
../../../src/Makefile.global:750: recipe for target 'gram.c' failed
make[2]: *** [gram.c] Error 1
make[2]: Leaving directory
'/var/lib/postgresql/git/postgresql/src/backend/parser'
Makefile:137: recipe for target 'parser/gram.h' failed
make[1]: *** [parser/gram.h] Error 2
make[1]: Leaving directory '/var/lib/postgresql/git/postgresql/src/backend'
src/Makefile.global:389: recipe for target 'submake-generated-headers'
failed
make: *** [submake-generated-headers] Error 2

* UPDATE doesn't set EndTime correctly, so err... the patch doesn't
> work on this aspect.
> Everything else does actually work, AFAICS, so we "just" need a way to
> update the END_TIME column in place...
> So input from other Hackers/Committers needed on this point to see
> what is acceptable.
>
> * Anomalies around use of CURRENT_TIMESTAMP are not discussed or resolved
>
> * No discussion, comments or tests around freezing and whether that
> causes issues here
>
> * What happens if you ask for a future time?
> It will give an inconsistent result as it scans, so we should refuse a
> query for time > current_timestamp.

* ALTER TABLE needs some work, it's a bit klugey at the moment and
> needs extra tests.
> Should refuse DROP COLUMN on a system time column
>
> * Do StartTime and EndTime show in SELECT *? Currently, yes. Would
> guess we wouldn't want them to, not sure what standard says.
>
>
I prefer to have them hidden by default. This was mentioned up-thread with
no decision, it seems the standard is ambiguous. MS SQL appears to
have flip-flopped on this decision [1].

> SELECT * shows the timestamp columns, don't we need to hide the period
> > timestamp columns from this query?
> >
> >
> The sql standard didn't dictate hiding the column but i agree hiding it by
> default is good thing because this columns are used by the system
> to classified the data and not needed in user side frequently. I can
> change to that if we have consensus

> * The syntax changes in gram.y probably need some coralling
>
> Overall, it's a pretty good patch and worth working on more. I will
> consider a recommendation on what to do with this.
>
> --
> Simon Riggs http://www.EnterpriseDB.com/

I am increasingly interested in this feature and have heard others asking
for this type of functionality. I'll do my best to continue reviewing and
testing.

Thanks,

Ryan Lambert

[1] https://bornsql.ca/blog/temporal-tables-hidden-columns/