On Mon, Jan 11, 2021 at 7:02 AM Simon Riggs <simon.ri...@enterprisedb.com> wrote:
> On Sat, Jan 9, 2021 at 10:39 AM Simon Riggs > <simon.ri...@enterprisedb.com> wrote: > > > > On Fri, Jan 8, 2021 at 9:19 PM Ryan Lambert <r...@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. Thanks, Ryan Lambert
