On Sat, Jan 9, 2021 at 10:39 AM Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > On Fri, Jan 8, 2021 at 9:19 PM Ryan Lambert <ryan@rustprooflabs.com> wrote: > > >> Updated v11 with additional docs and some rewording of messages/tests > >> to use "system versioning" correctly. > >> > >> No changes on the points previously raised. > >> > > Thank you! The v11 applies and installs. I tried a simple test, unfortunately it appears the versioning is not working. The initial value is not preserved through an update and a new row does not appear to be created. > > Agreed. I already noted this in my earlier review comments.
I'm pleased to note that UPDATE-not-working was a glitch, possibly in an earlier patch merge. That now works as advertised.
It is working as expected now, Thank you!
I've added fairly clear SGML docs to explain how the current patch works, which should assist wider review.
The default column names changed to start_timestamp and end_timestamp. A number of places in the docs still refer to StartTime and EndTime. I prefer the new names without MixedCase.
Also moved test SQL around a bit, renamed some things in code for readability, but not done any structural changes.
This is looking much better now... with the TODO/issues list now looking like this...
* Anomalies around use of CURRENT_TIMESTAMP are not discussed or resolved. Probably need to add a test that end_timestamp > start_timestamp or ERROR, which effectively enforces serializability.
* 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, but currently doesn't
* UPDATE foo SET start_timestamp = DEFAULT should fail but currently doesn't
* Do StartTime and EndTime show in SELECT *? Currently, yes. Would guess we wouldn't want them to, not sure what standard says.
From here, the plan would be to set this to "Ready For Committer" in about a week. That is not the same thing as me saying it is ready-for-commit, but we need some more eyes on this patch to decide if it is something we want and, if so, are the code changes cool.
Should I invest time now into further testing with more production-like scenarios on this patch? Or would it be better to wait on putting effort into that until it has had more review? I don't have much to offer for help on your current todo list.