sql: allow relative AS OF queries with a negative interval

[maddyblue]

Accept an interval type or a string with a valid interval. The interval
is added to the statement's statement_timestamp() time. This can be
used to run relative AOST queries without calculating the time in
the constant.

Fixes #16424

Release note: AS OF SYSTEM TIME queries now accept a negative interval
to produce a relative time from the statement's statement_timestamp()
time.

[cockroach-teamcity]

This change is 

[knz]

The code is OK but the commit message is incorrect: you are using a computation relative to statement_timestamp(), not now. (now is the txn timestamp).

I say "the code is OK" but did you have any reason to use the statement timestamp instead of the node's current HLC timestamp? They are not the same and I think we must use the HLC timestamp if we want to enable clients to guarantee to be able to read their own writes by other txns.

Beyond that a question not related to this PR: does this make sense to allow any arbitrary ASOT timestamp? It seems to me that it's only meaningful if the ts is in the past and at least MaxOffset old?

Also nit: place the text "Fixes #..." above the release not -- everything after the "release note" prefix goes into the release note.

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[maddyblue]

I changed the code to use the hlc Clock.Now() instead of the statement_timestamp() and it broke because when verifying that the timestamp was ok in the planner.getTimestamp func. This happens because the hlc clock is called during the verification of the AOST timestamp and then later when creating the txn. The hlc clock moves since it is called twice and so fails the call. During prepared and not prepared statements the txn timestamp is set differently, and I didn't see an easy way to correctly set this time to be something that wouldn't change. So I think it's ok to use the statement_timestamp time from the evalCtx.

I agree about your assessment about appropriate times, but don't know what to do about it.

---

Review status: 2 of 3 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[knz]

Ok then you can keep the code as-is but please correct the commit message to mention the statement timestamp. LGTM after that.

[maddyblue]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)

[danhhz]

Nice, does this also work with BACKUP? If so, we should probably add it to the docs, since it's currently quite manual to construct the timestamp from 10 seconds ag" that we recommend: https://www.cockroachlabs.com/docs/stable/backup.html#performance

[maddyblue]

Yes, it should work with backup.