sem/tree: use StmtTimestamp as max for AsOf, enforce non-zero intervals #34547

ajwerner · 2019-02-04T23:06:59Z

Before this PR there was some unexpected behavior when using AOST clauses with
literals which were very small. There are two culprits. The first is that the
logic to evaluate the AOST took a maximum value that was different and always
greater than the statement time which is the timestamp from which intervals
literals are used to compute a timestamp. That means that if the interval
literal were positive but smaller than the difference between the "max" and
the statement time, they would be judged to be valid. There's no reason we need
to compute an additional maximum, instead we just use statement time as the
maximum. Another issue is that values smaller than 1 microsecond are rounded to
zero so 100ns, 0,0 and -100ns are equivalent as far as the evaluation
code was concerned. This diff makes all of these intervals which are rounded to
zero illegal.

Fixes #34371.

Release note (sql change): Disallow AS OF SYSTEM TIME interval expressions
less than 1 microsecond in the past.

cockroach-teamcity · 2019-02-04T23:07:10Z

This change is

ajwerner · 2019-02-05T00:31:54Z

I messed something up here, will dig more.

ajwerner · 2019-02-07T01:23:03Z

This is ready to go PTAL when you get a chance.

maddyblue · 2019-02-13T18:22:41Z

pkg/sql/logictest/testdata/logic_test/as_of

@@ -57,3 +52,20 @@ query T
 SELECT * FROM (SELECT now()) AS OF SYSTEM TIME '2018-01-01'
 ----
 2018-01-01 00:00:00 +0000 UTC
+
+# Verify that zero intervals those indistinguishable from zero cause an error.


I think this is missing a word.

or rather had one too many

maddyblue · 2019-02-13T18:26:32Z

pkg/sql/sem/tree/as_of.go

@@ -97,7 +97,11 @@ func EvalAsOfTimestamp(
 		}
 		// Attempt to parse as an interval.
 		if iv, err := ParseDInterval(s); err == nil {


ParseDInterval is for parsing strings into postgres-compatible SQL datums, which happen to not support nanoseconds. We should consider changing this function to one that does support nanos, since that's a valid argument to AOST. We could just use go's time.ParseDuration for that, which would lose us support for days and months, but that probably doesn't matter since most GC intervals are less than that. Then again, why would anyone use AOST with a very specific nanosecond argument. Like, I can't think of a situation where that would actually matter. Ok so maybe this is fine? Just some thoughts.

knz

Reviewed 20 of 20 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @mjibson)

pkg/sql/sem/tree/as_of.go, line 99 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

ParseDInterval is for parsing strings into postgres-compatible SQL datums, which happen to not support nanoseconds. We should consider changing this function to one that does support nanos, since that's a valid argument to AOST. We could just use go's time.ParseDuration for that, which would lose us support for days and months, but that probably doesn't matter since most GC intervals are less than that. Then again, why would anyone use AOST with a very specific nanosecond argument. Like, I can't think of a situation where that would actually matter. Ok so maybe this is fine? Just some thoughts.

I too was concerned by this. However maybe it's OK if SQL intervals don't support nanoseconds. Also it would be troublesome to have a feature that has one precision in one context, and another in a different context. It would make things difficult to explain. However this is fine only as long as we offer another mechanism for AOST to work with nanoseconds instead.

Hence my question: do we properly support arithmetic using decimals in this context, and are we always converting a decimal value to nanoseconds (like we used to)?

If that's the case:

I am not too bothered by intervals not supporting nanoseconds
however, I think that we need to test the nanosecond behavior. Please convert the existing nanosecond tests to use decimals instead and verify that sub-microseconds intervals are properly supported.

ajwerner · 2019-02-14T12:35:03Z

@knz I’m not sure I understand. The decimal literal is used here to specify a specific hlc time stamp rather than an interval or relative offset. I do believe it supports nanosecond resolution and will add testing to that effect.

I do not however believe that there exists a nanosecond resolution mechanism to deal with relative offsets from now. Given that, I’m not totally sure how to update the old ns tests. Am I missing something?

knz · 2019-02-14T13:27:15Z

The decimal literal is used here to specify a specific hlc time stamp rather than an interval or relative offset. I do believe it supports nanosecond resolution and will add testing to that effect.

What about an expression with type DECIMAL. Convert now() to DECIMAL to get the current timestamp as nanosecond, perform arithmetic on that to compute a nanosecond offset, etc. Or handle negative decimals as relative to the current timestamp automatically.

I think we can do ns-precision relative offsetting this way, isn't that the case?

ajwerner · 2019-02-14T13:39:19Z

What about an expression with type DECIMAL. Convert now() to DECIMAL to get the current timestamp as nanosecond, perform arithmetic on that to compute a nanosecond offset, etc.

This is disallowed by the constant expression requirement.

Or handle negative decimals as relative to the current timestamp automatically.

This seems doable. I wonder if it's confusing for users as we explicitly say in the docs that the decimal format is used for HLC.

ajwerner · 2019-02-14T13:43:11Z

Just to clarify, if we move forward with the vision of using negative decimal values as offsets we still need to nail down the semantics. For the decimal HLC values the integer portion represents time in unix nanos and the decimal portion represents the logical clock. Would you be in a favor of a solution that had similar semantics with regards to the wall clock portion of the decimal and ignored the logical portion?

e.g.
-1: 1 ns
-1.111: 1 ns
-1001.111: 1001 ns
etc?

knz · 2019-02-14T13:44:28Z

My idea with decimal expressions is that the value would get evaluated as usual, and then whatever decimal portion remains becomes the logical part (without truncation). If the user wants truncation they can do that manually.

ajwerner · 2019-02-14T13:46:12Z

My idea with decimal expressions is that the value would get evaluated as usual, and then whatever decimal portion remains becomes the logical part (without truncation). If the user wants truncation they can do that manually.

Sounds good. I'll type it up.

ajwerner · 2019-02-14T14:38:25Z

Updated, PTAL

maddyblue

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @danhhz, @knz, and @mjibson)

pkg/sql/sem/tree/as_of.go, line 122 at r2 (raw file):

		// preserving the specified logical value.
		if convErr == nil && ts.WallTime < 0 {
			ts.WallTime = stmtTimestamp.Add(time.Duration(ts.WallTime)).UnixNano()

The semantics of this are odd. It subtracts the walltimes but leaves the logical as is. Also now 0 means two different things (unix epoch or now) depending on which way you go from it. This makes me minorly uncomfortable even though it is a nice solution to one problem. @bdarnell do you have thoughts on this choice?

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @knz, and @mjibson)

pkg/sql/sem/tree/as_of.go, line 122 at r2 (raw file):

Also now 0 means two different things (unix epoch or now) depending on which way you go from it.

Zero still means unix epoch.

The semantics of this are odd. It subtracts the walltimes but leaves the logical as is.

I agree, I too was at a loss for how to deal with the logical portion of the decimal when Raphael suggested interpreting negative decimals as offsets. I just went with his suggestion here.

My idea with decimal expressions is that the value would get evaluated as usual, and then whatever decimal portion remains becomes the logical part (without truncation). If the user wants truncation they can do that manually.

#34547 (comment)

maddyblue

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @danhhz, @knz, and @mjibson)

pkg/sql/sem/tree/as_of.go, line 122 at r2 (raw file):

Previously, ajwerner wrote…

Also now 0 means two different things (unix epoch or now) depending on which way you go from it.

Zero still means unix epoch.

The semantics of this are odd. It subtracts the walltimes but leaves the logical as is.

I agree, I too was at a loss for how to deal with the logical portion of the decimal when Raphael suggested interpreting negative decimals as offsets. I just went with his suggestion here.

My idea with decimal expressions is that the value would get evaluated as usual, and then whatever decimal portion remains becomes the logical part (without truncation). If the user wants truncation they can do that manually.

#34547 (comment)

I mean the idea of zero, not it's actual value. 0 + X = unix epoch + X. 0 - X = now - X.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @knz, and @mjibson)

pkg/sql/sem/tree/as_of.go, line 122 at r2 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I mean the idea of zero, not it's actual value. 0 + X = unix epoch + X. 0 - X = now - X.

The only counterpoint I'd make to that is that positive values of X in roughly [1, 1388448000000000000] aren't useful

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @knz, and @mjibson)

pkg/sql/sem/tree/as_of.go, line 122 at r2 (raw file):

Previously, ajwerner wrote…

The only counterpoint I'd make to that is that positive values of X in roughly [1, 1388448000000000000] aren't useful

If we explicitly disallow using this syntax to create a timestamp before say, 2000, does that make the discontinuity more palatable?

maddyblue

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @danhhz, @knz, and @mjibson)

pkg/sql/sem/tree/as_of.go, line 122 at r2 (raw file):

Previously, ajwerner wrote…

If we explicitly disallow using this syntax to create a timestamp before say, 2000, does that make the discontinuity more palatable?

I'm not concerned about the discontinuity. Just that 0 as a reference point is now two different things depending on context. I'm not sure it's bad, just pointing it out. Will it confuse users? Will it cause accidental, unknown, or incorrect usage? When we introduced negative intervals that was a new feature since the interval type previously did nothing. This one changes the existing meaning of int and decimal types, which is an explicit change in the current behavior (although clearly a totally silly one since who wants data before the unix epoch). Because it's a change instead of a new behavior I feel ok at least having a ponder about if it's a good design decision.

knz · 2019-02-14T21:46:58Z

This discussion is swerving outside of the scope of a code review into a design question. The review context is insufficient to resolve. I recommend to write up in prose what are the few problems we're trying to solve here and enumerate the alternatives, with pros and cons for each. This will provide clarity and perhaps reveal a new way we haven't considered yet. Just my 2cents. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

ajwerner · 2019-02-14T22:47:43Z

Okay, here's a recap. AOST currently permits strings like '0', '0,0', '1ns', '1us', '-1ns', '1100ns' where the semantics on all of those examples are confusing and the positive values are likely bugs. All of the above strings are interpreted as time intervals. The large (larger than 1us) intervals were permitted due to the fact that before this change we enforced that the evaluated timestamp was not historical by plumbing a max timestamp value into the evaluation. On most code paths this max was after the statement time. Rectifying this was straightforward and simplified the interface. The next problem is that our interval representation only has microsecond precision. This means '999ns' and '-1ns' are indistinguishable from zero. The original change, before review feedback accepted this and enforced that AOST expressions which are relative to the statement time must be at least 1us in the past.

@knz suggested that there should be a way to express AOST relative to the statement time at nanosecond precision (something not currently supported). My understanding of the source of this suggestion is that testing code today currently uses an AOST '-1ns' to indicate that now should be used as the time. Prior to the most recent iteration I had changed the testing call sites to instead pass -1000ns (which could be written -1us). Raphael suggested that one way to do add support for nanosecond precision offsets would be to interpret negative decimal and integer values and I obliged.

I do not feel that having support for nanosecond precision relative offsets is important but I do not feel strongly about that.

ajwerner · 2019-02-14T23:07:08Z

If a user wants to use "5 nanoseconds in the past" they can write this: AS OF SYSTEM TIME now()::DECIMAL * 1e9 - 5

You brought up a point I failed to mention. AOST expressions have to be constant and thus both now and the arithmetic is disallowed.

knz · 2019-02-14T23:08:18Z

Oh hm. that's a bummer.

knz · 2019-02-14T23:09:04Z

(I may need a little more time to think this over. Sorry that it's late in my TZ so my brain doesn't function any more for today.)

knz · 2019-02-14T23:11:52Z

Ok so a possible alternative would be to split case 3 in my last comment:

decimal expression/value, strictly positive: take as absolute timestamp
decimal expression/value, strictly negative: take relative to now()

knz · 2019-02-14T23:12:39Z

(note that the value 0 itself would be disallowed in both cases, to avoid the discontinuity)

bdarnell

Here's another wrinkle: AOST values between now() and now() - max_offset are kind of unsafe because they are within the uncertainty window but disable the checks for ReadWithinUncertaintyIntervalError. I think we may actually want to ban AOST values in this range, and since max_offset will generally be measured in milliseconds, there's no need for nanosecond precision in negative intervals.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @danhhz, @knz, and @mjibson)

knz · 2019-02-14T23:24:45Z

they are within the uncertainty window but disable the checks for ReadWithinUncertaintyIntervalError.

I recall a convo either on the forum or gitter where that was exactly the feature looked for by a user: disable the uncertainty error at the risk of non-serializable reads, where the goal was to get a guaranteed-bound read latency on recent data with a weaker consistency expectation.

I wouldn't be keen to disallow that time interval outright before we double-check that it's not useful (and/or find out other ways to achieve the same).

ajwerner · 2019-02-14T23:25:26Z

Here's another wrinkle: AOST values between now() and now() - max_offset are kind of unsafe because they are within the uncertainty window but disable the checks for ReadWithinUncertaintyIntervalError. I think we may actually want to ban AOST values in this range, and since max_offset will generally be measured in milliseconds, there's no need for nanosecond precision in negative intervals.

That's a concern that we certainly had not considered. I'd prefer to push that into a separate issue is that's okay. For now I think I'll revert this change to where it stood this morning where the tests were changed from -1ns to -1us and otherwise not change the semantics further.

knz · 2019-02-14T23:27:48Z

I think this conversation was long-winded and it's the end of the day. We should at least let one night worth of sleep precipitate the various thoughts in motion, before conclusions are drawn.

knz · 2019-02-22T14:56:02Z

LGTM thanks

ajwerner · 2019-02-22T15:23:50Z

TFTR

ajwerner · 2019-02-22T15:23:54Z

bors r+

craig · 2019-02-22T15:23:56Z

👎 Rejected by PR status

ajwerner · 2019-02-22T15:25:04Z

bors r+

craig · 2019-02-22T15:46:40Z

Build failed

GitHub CI (Cockroach)

Before this PR there was some unexpected behavior when using AOST clauses with literals which were very small. There are two culprits. The first is that the logic to evaluate the AOST took a maximum value that was different and always greater than the statement time which is the timestamp from which intervals literals are used to compute a timestamp. That means that if the interval literal were positive but smaller than the difference between the "max" and the statement time, they would be judged to be valid. There's no reason we need to compute an additional maximum, instead we just use statement time as the maximum. Another issue is that values smaller than 1 microsecond are rounded to zero so `100ns`, `0,0` and `-100ns` are equivalent as far as the evaluation code was concerned. This diff makes all of these intervals which are rounded to zero illegal. Note that in a number of testing scenarios, particularly around backup/restore and automatic stats collection we used to use a zero as of system time value which this PR changes to 1 microsecond which ought to be functionally equivalent. Fixes cockroachdb#34371. Release note (sql change): Disallow `AS OF SYSTEM TIME` interval expressions less than 1 microsecond in the past.

ajwerner · 2019-02-22T19:22:13Z

bors r+

craig · 2019-02-22T19:48:39Z

Build failed

GitHub CI (Cockroach)

ajwerner · 2019-02-22T21:21:39Z

bors r+

34547: sem/tree: use StmtTimestamp as max for AsOf, enforce non-zero intervals r=ajwerner a=ajwerner Before this PR there was some unexpected behavior when using AOST clauses with literals which were very small. There are two culprits. The first is that the logic to evaluate the AOST took a maximum value that was different and always greater than the statement time which is the timestamp from which intervals literals are used to compute a timestamp. That means that if the interval literal were positive but smaller than the difference between the "max" and the statement time, they would be judged to be valid. There's no reason we need to compute an additional maximum, instead we just use statement time as the maximum. Another issue is that values smaller than 1 microsecond are rounded to zero so `100ns`, `0,0` and `-100ns` are equivalent as far as the evaluation code was concerned. This diff makes all of these intervals which are rounded to zero illegal. Fixes #34371. Release note (sql change): Disallow `AS OF SYSTEM TIME` interval expressions less than 1 microsecond in the past. Co-authored-by: Andrew Werner <[email protected]>

craig · 2019-02-22T21:44:00Z

Build succeeded

GitHub CI (Cockroach)

ajwerner requested review from danhhz and a team February 4, 2019 23:06

ajwerner requested a review from a team as a code owner February 4, 2019 23:07

ajwerner requested review from a team February 4, 2019 23:07

ajwerner force-pushed the ajwerner/as-of-always-in-the-past branch 2 times, most recently from 29679ae to d2dafa5 Compare February 6, 2019 23:23

maddyblue approved these changes Feb 13, 2019

View reviewed changes

knz reviewed Feb 14, 2019

View reviewed changes

ajwerner force-pushed the ajwerner/as-of-always-in-the-past branch from d2dafa5 to 0b12d5b Compare February 14, 2019 14:09

maddyblue suggested changes Feb 14, 2019

View reviewed changes

ajwerner commented Feb 14, 2019

View reviewed changes

maddyblue suggested changes Feb 14, 2019

View reviewed changes

ajwerner commented Feb 14, 2019

View reviewed changes

maddyblue suggested changes Feb 14, 2019

View reviewed changes

bdarnell reviewed Feb 14, 2019

View reviewed changes

ajwerner force-pushed the ajwerner/as-of-always-in-the-past branch 2 times, most recently from 617d447 to 73a20f3 Compare February 15, 2019 18:46

maddyblue approved these changes Feb 21, 2019

View reviewed changes

ajwerner force-pushed the ajwerner/as-of-always-in-the-past branch from 73a20f3 to dddffb9 Compare February 22, 2019 15:24

ajwerner force-pushed the ajwerner/as-of-always-in-the-past branch from dddffb9 to 8d9c588 Compare February 22, 2019 16:31

craig bot merged commit 8d9c588 into cockroachdb:master Feb 22, 2019

maddyblue mentioned this pull request Mar 5, 2019

roachtest: backup: use AOST time slightly in the past #35359

Merged

sem/tree: use StmtTimestamp as max for AsOf, enforce non-zero intervals #34547

sem/tree: use StmtTimestamp as max for AsOf, enforce non-zero intervals #34547

Conversation

ajwerner commented Feb 4, 2019

cockroach-teamcity commented Feb 4, 2019

ajwerner commented Feb 5, 2019

ajwerner commented Feb 7, 2019

maddyblue Feb 13, 2019

Choose a reason for hiding this comment

ajwerner Feb 14, 2019

Choose a reason for hiding this comment

maddyblue Feb 13, 2019

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ajwerner commented Feb 14, 2019

knz commented Feb 14, 2019

ajwerner commented Feb 14, 2019

ajwerner commented Feb 14, 2019

knz commented Feb 14, 2019

ajwerner commented Feb 14, 2019

ajwerner commented Feb 14, 2019

maddyblue left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

maddyblue left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

maddyblue left a comment

Choose a reason for hiding this comment

knz commented Feb 14, 2019 via email

ajwerner commented Feb 14, 2019

ajwerner commented Feb 14, 2019

knz commented Feb 14, 2019

knz commented Feb 14, 2019

knz commented Feb 14, 2019

knz commented Feb 14, 2019 • edited Loading

bdarnell left a comment

Choose a reason for hiding this comment

knz commented Feb 14, 2019

ajwerner commented Feb 14, 2019

knz commented Feb 14, 2019

knz commented Feb 22, 2019

ajwerner commented Feb 22, 2019

ajwerner commented Feb 22, 2019

craig bot commented Feb 22, 2019

ajwerner commented Feb 22, 2019

craig bot commented Feb 22, 2019

Build failed

ajwerner commented Feb 22, 2019

craig bot commented Feb 22, 2019

Build failed

ajwerner commented Feb 22, 2019

craig bot commented Feb 22, 2019

Build succeeded

knz commented Feb 14, 2019 •

edited

Loading