sql: support AS OF SYSTEM TIME readonly queries

[maddyblue]

This is implemented by adding special logic to the executor. It is
unfortunate that we have to teach it so much about how SELECT statements
work, but the executor is the thing that can set the proto timestamps
during each retry attempt (they cannot only be set once since the
auto-retry stuff will bump the timestamps otherwise).

In the case of an AS OF query, we only want to fetch the table descriptor
at that time, and not a lease, even if the time is current.

---

This change is

[tbg]

Looks good generally.

Previously, mjibson (Matt Jibson) wrote…

sql: support AS OF SYSTEM TIME readonly queries

This is implemented by adding special logic to the executor. It is

unfortunate that we have to teach it so much about how SELECT statements

work, but the executor is the thing that can set the proto timestamps

during each retry attempt (they cannot only be set once since the

auto-retry stuff will bump the timestamps otherwise).

In the case of an AS OF query, we only want to fetch the table descriptor

at that time, and not a lease, even if the time is current.

---

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

sql/as_of_test.go, line 50 [r1] (raw file):

  var n1, n2 time.Time

  // Expect an error if table doesn't exist at time.

I don't understand this comment.

---

sql/as_of_test.go, line 57 [r1] (raw file):

  if _, err := db.Query(fmt.Sprintf("SELECT a FROM d.t AS OF SYSTEM TIME '%s'", d.String())); err == nil {
      t.Fatal("expected error")
  } else if err.Error() != `pq: database "d" does not exist` {

Use testutils.IsError().

It would also be interesting to run this select after you create the table (but you should still get an error because the db/table did not exist at n1). Ideally testing both cases (db and table).

Can you sprinkle commentary through this test and avoid overwriting variables (instead naming them better? Something like tsPreCreate, tsPostCreate, tsPostInsert, tsPostUpdate, ...)

---

sql/as_of_test.go, line 126 [r1] (raw file):

  if err := db.QueryRow("SELECT a FROM d.t").Scan(&i); err == nil {
      t.Fatal("expected error")
  } else if err.Error() != `pq: qualified name "a" not found` {

testutils.IsError

---

sql/as_of_test.go, line 162 [r1] (raw file):

  }

  var m int64

Why not name this and everything else here more descriptively?

---

sql/as_of_test.go, line 188 [r1] (raw file):

              if count > 0 && bytes.Contains(req.Key, []byte(key)) {
                  magicVals.restartCounts[key]--
                  err := roachpb.NewTransactionPushError(*hdr.Txn)

TransactionPushError isn't what you want. You want a TransactionRetryError carrying a copy of the incoming transaction whose timestamp has been pushed (I suggest pushing the logical component only to avoid funny business), and asserting here that the incoming transaction always has as the correct deadline (even if you let it commit).

---

sql/executor.go, line 407 [r1] (raw file):

      // can be consumed.
      stmtsToExec := stmts
      var protoTS *roachpb.Timestamp

Add comment.

---

sql/executor.go, line 461 [r1] (raw file):

          if protoTS != nil {
              txnState.txn.Proto.OrigTimestamp = *protoTS
              txnState.txn.Proto.MaxTimestamp = *protoTS

You need to set the deadline. Without this, the suggested updates to the test above should expose this as a bug.

---

sql/planner.go, line 58 [r1] (raw file):

  // have already been set correctly, and this flag is used in layers below
  // the executor to modify the behavior of the SELECT. For example the table
  // descriptor is not leased, only fetched at the correct time.

So there's no caching of the table descriptor? That sounds like something we're going to have to track (and eventually fix). Could you file it before this merges?

---

sql/scan.go, line 253 [r1] (raw file):

  var err error

  if p.asOf {

A comment wouldn't hurt.

---

Comments from Reviewable

[maddyblue]

Review status: 7 of 10 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

---

sql/as_of_test.go, line 50 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

I don't understand this comment.

Done.

---

sql/as_of_test.go, line 57 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Use testutils.IsError().

It would also be interesting to run this select after you create the table (but you should still get an error because the db/table did not exist at n1). Ideally testing both cases (db and table).

Can you sprinkle commentary through this test and avoid overwriting variables (instead naming them better? Something like tsPreCreate, tsPostCreate, tsPostInsert, tsPostUpdate, ...)

Done.

---

sql/as_of_test.go, line 126 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

testutils.IsError

Done.

---

sql/as_of_test.go, line 162 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Why not name this and everything else here more descriptively?

Done.

---

sql/as_of_test.go, line 188 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

TransactionPushError isn't what you want. You want a TransactionRetryError carrying a copy of the incoming transaction whose timestamp has been pushed (I suggest pushing the logical component only to avoid funny business), and asserting here that the incoming transaction always has as the correct deadline (even if you let it commit).

I have changed the error to this type but the deadline bugs you say should happen are not. I'm not sure what you mean exactly by "carrying a copy". `NewErrorWithTxn` is called a few lines down which is what I think you mean. If that is correct, then I'm not sure why the deadline error isn't triggering.

---

sql/executor.go, line 407 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Add comment.

Done.

---

sql/planner.go, line 58 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

So there's no caching of the table descriptor? That sounds like something we're going to have to track (and eventually fix). Could you file it before this merges?

Filed #7143.

---

sql/scan.go, line 253 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

A comment wouldn't hurt.

Done.

---

Comments from Reviewable

[knz]

Reviewed 1 of 10 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/executor.go, line 1173 [r2] (raw file):

func isAsOf(planMaker *planner, stmt parser.Statement, max roachpb.Timestamp) (*roachpb.Timestamp, error) {
  s, ok := stmt.(*parser.Select)

I'd suggest adding a comment here "Analysis of the select statement is done here to bypass the logic in newPlan(), since that requires the transaction to be started already."

---

Comments from Reviewable

[tbg]

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

sql/as_of_test.go, line 188 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

I have changed the error to this type but the deadline bugs you say should happen are not. I'm not sure what you mean exactly by "carrying a copy". NewErrorWithTxn is called a few lines down which is what I think you mean. If that is correct, then I'm not sure why the deadline error isn't triggering.

You're using a `TransactionRetryError`, but you're not doing the rest (populating it with a txn that carries a higher timestamp). You must make a copy of `args.Hdr.Txn` and modify it as suggested before returning it with the error - you're currently returning the same one, so no increased timestamp.

---

sql/as_of_test.go, line 104 [r2] (raw file):

      t.Fatalf("expected %v, got %v", val1, i)
  }
  if err := db.QueryRow("SELECT a FROM d.t").Scan(&i); err != nil {

This is already done above. Is there a particular reason to have this twice?

---

sql/as_of_test.go, line 117 [r2] (raw file):

  }

  // Future queries shouldn't work.

What about reading at a past timestamp which has since been GC'ed? Probably needs a little more setup (I think writing a timestamp into the last-gc'ed field when the first range is bootstrapped is enough, given that we copy that information on a split (right?)); then you should be able to use a time like '1970-01-01' to trigger the error. Just to make sure that it fails in the way that we think. Maybe that error needs to be converted to some error prescribed by the SQL standard, too (if an adequate counterpart exists).

---

sql/as_of_test.go, line 140 [r2] (raw file):

      t.Fatalf("expected %v, got %v", val1, i)
  }
  if err := db.QueryRow("SELECT a FROM d.t").Scan(&i); err == nil {

What does this test?

---

sql/as_of_test.go, line 178 [r2] (raw file):

  }

  var logical int64

I think a better name for this would be walltime. logical makes me think of the logical portion of the HLC timestamps, but that is precisely what you're throwing away here (by casting the decimal to an int).

---

Comments from Reviewable

[maddyblue]

Review status: 8 of 10 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

sql/as_of_test.go, line 104 [r2] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

This is already done above. Is there a particular reason to have this twice?

Nope. Removed.

---

sql/as_of_test.go, line 117 [r2] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

What about reading at a past timestamp which has since been GC'ed? Probably needs a little more setup (I think writing a timestamp into the last-gc'ed field when the first range is bootstrapped is enough, given that we copy that information on a split (right?)); then you should be able to use a time like '1970-01-01' to trigger the error. Just to make sure that it fails in the way that we think. Maybe that error needs to be converted to some error prescribed by the SQL standard, too (if an adequate counterpart exists).

Instead of triggering a GC, I'm using a pre-1970 timestamp, which appears to work.

---

sql/as_of_test.go, line 140 [r2] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

What does this test?

My thought was to verify that we get that expected error in the present after the other expected error from the past. Not strictly needed I suppose. Removed.

---

sql/as_of_test.go, line 178 [r2] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

I think a better name for this would be walltime. logical makes me think of the logical portion of the HLC timestamps, but that is precisely what you're throwing away here (by casting the decimal to an int).

Done.

---

sql/executor.go, line 461 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You need to set the deadline. Without this, the suggested updates to the test above should expose this as a bug.

I did a bunch of tests and am not convinced about this. The deadline checking examines the `Timestamp` field of the proto. Since (as of this most recent revision) I am setting `.Timestamp` at the start of each retry, I do not see how it could ever trigger the deadline failure. Indeed, with the modifications to the test suggested above, commenting out the deadline setting in executor.go doesn't change the output of the test. I have left the deadline setting in, because I don't think it does any harm and could potentially find problems in the future. But I'm not sure it's doing anything explicitly useful right now.

---

sql/executor.go, line 1173 [r2] (raw file):

Previously, knz (kena) wrote…

I'd suggest adding a comment here "Analysis of the select statement is done here to bypass the logic in newPlan(), since that requires the transaction to be started already."

Done.

---

Comments from Reviewable

[maddyblue]

Review status: 8 of 10 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

sql/as_of_test.go, line 188 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You're using a TransactionRetryError, but you're not doing the rest (populating it with a txn that carries a higher timestamp). You must make a copy of args.Hdr.Txn and modify it as suggested before returning it with the error - you're currently returning the same one, so no increased timestamp.

Done, I think.

---

Comments from Reviewable

[andreimatei]

Review status: 8 of 10 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

sql/executor.go, line 421 [r3] (raw file):

              // Check for AS OF SYSTEM TIME. If it is present but not detected here,
              // it will raise an error later on.
              protoTS, err = isAsOf(planMaker, stmtsToExec[0], execOpt.MinInitialTimestamp)

how about txnTimestamp?

---

sql/executor.go, line 421 [r3] (raw file):

              // Check for AS OF SYSTEM TIME. If it is present but not detected here,
              // it will raise an error later on.
              protoTS, err = isAsOf(planMaker, stmtsToExec[0], execOpt.MinInitialTimestamp)

is there a point in supporting txn retries for these reads, given that we can't update the txn timestamp?
How about verifying that we're below the uncertainty period, and then asserting that there's not retries?

---

sql/executor.go, line 465 [r3] (raw file):

              txnState.txn.Proto.OrigTimestamp = *protoTS
              txnState.txn.Proto.MaxTimestamp = *protoTS
              txnState.txn.UpdateDeadlineMaybe(*protoTS)

assert on the result of UpdateDeadlineMaybe (or introduce a result if there isn't one). Cause this code doesn't like the Maybe aspect :)

---

sql/executor.go, line 1176 [r3] (raw file):

// isAsOf analyzes a select statement to bypass the login in newPlan(),
// since that requires the transaction to be started already.
func isAsOf(planMaker *planner, stmt parser.Statement, max roachpb.Timestamp) (*roachpb.Timestamp, error) {

please comment the return val

---

sql/executor.go, line 1217 [r3] (raw file):

  }
  if max.Less(ts) {
      return nil, fmt.Errorf("cannot specify timestamp in the future")

consider add an ErrorWithPGCode, otherwise this will result in an internal error being returned to the client

---

sql/planner.go, line 56 [r3] (raw file):

  // If set, this is an AS OF SYSTEM TIME query. The transaction timestamps
  // have already been set correctly, and this flag is used in layers below

this comment is a bit weird. Not clear what transaction timestamps it's talking about.

---

Comments from Reviewable

[maddyblue]

Review status: 8 of 10 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

sql/executor.go, line 421 [r3] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

how about txnTimestamp?

Because that uses the `PhysicalClock`, and we want the HLC clock.

---

sql/executor.go, line 421 [r3] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

is there a point in supporting txn retries for these reads, given that we can't update the txn timestamp?
How about verifying that we're below the uncertainty period, and then asserting that there's not retries?

I think retries can occur if there's an old intent, say, in a long-running and still open transaction.

---

sql/executor.go, line 465 [r3] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

assert on the result of UpdateDeadlineMaybe (or introduce a result if there isn't one). Cause this code doesn't like the Maybe aspect :)

I did that and it causes a bug. The function returns true if it set the deadline, which it does only if it's strictly older than the previous deadline. Thus, setting the same deadline more than 1 times will return false for all times after the first. It's not a useful thing to check: it's not an error if it's not set.

---

sql/executor.go, line 1176 [r3] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

please comment the return val

Done.

---

sql/executor.go, line 1217 [r3] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

consider add an ErrorWithPGCode, otherwise this will result in an internal error being returned to the client

I considered it and have decided to not. The best error I could come up with is 22000, a generic "data exception", but I have no idea if that's an appropriate use of that error or not.

---

sql/planner.go, line 56 [r3] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

this comment is a bit weird. Not clear what transaction timestamps it's talking about.

Agree. I removed the note about transaction timestamps since it's not needed.

---

Comments from Reviewable

[andreimatei]

LGTM

Previously, tschottdorf (Tobias Schottdorf) wrote…

Looks good generally.

---

Review status: 3 of 10 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

sql/executor.go, line 421 [r3] (raw file):

Previously, mjibson (Matt Jibson) wrote…

I think retries can occur if there's an old intent, say, in a long-running and still open transaction.

ah, yes, old intents. Although we saw that the read would actually get blocked and wait for those intents with the current code. In any case, I would think it's OK to just fail the historical read query in that case saying "history is still being written" and not bother with retries? Or maybe completely ignore intents that cannot be resolved, if that's a thing?

---

Comments from Reviewable

[maddyblue]

Review status: 3 of 10 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

---

sql/executor.go, line 421 [r3] (raw file):

Previously, andreimatei (Andrei Matei) wrote…

ah, yes, old intents. Although we saw that the read would actually get blocked and wait for those intents with the current code. In any case, I would think it's OK to just fail the historical read query in that case saying "history is still being written" and not bother with retries? Or maybe completely ignore intents that cannot be resolved, if that's a thing?

I'm wrong. I forgot that they blocked lower down. In that case I am not sure what could theoretically cause a retry. In the case of old intents, it still sits there and blocks. That behavior is ok for now, as a user can ctrl+c out if the want. I agree we should add some kind of error or flag the store to not spin or something for this case.

Toby may know what can cause a retry to occur.

---

Comments from Reviewable

[bdarnell]

Review status: 3 of 10 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

sql/as_of_test.go, line 56 [r5] (raw file):

  }
  tsEmpty := tm.Format(time.RFC3339Nano)
  if _, err := db.Query(fmt.Sprintf("SELECT a FROM d.t AS OF SYSTEM TIME '%s'", tsEmpty)); !testutils.IsError(err, `pq: database "d" does not exist`) {

Can we support placeholders here? As soon as #7172 is merged safesql should start complaining about this.

---

sql/as_of_test.go, line 149 [r5] (raw file):

      t.Fatal("unexpected error:", err)
  }
}

Add a test for SELECT now() AS OF SYSTEM TIME '...'. (It returns the "as of" time and not the real time, right?)

It would be nice to support expressions like AS OF SYSTEM TIME now() - interval '1h' but it makes my head hurt to think about now() evaluating differently in different parts of the same statement.

---

Comments from Reviewable

[maddyblue]

Review status: 3 of 10 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

sql/as_of_test.go, line 56 [r5] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Can we support placeholders here? As soon as #7172 is merged safesql should start complaining about this.

No, we can't support placeholders. The query needs to be type-checked based on the table descriptor at the "as of" time. Type check is performed during the prepare phase to infer the placeholder types, and thus needs access to a table descriptor. But since we don't know the time at which we need the table descriptor, the current table descriptor is leased and used. Thus, if you try to read a column in the past that has been deleted in the future (which is valid, and we have a test to do so), then that test will fail if it's using a placeholder because it looks for the column in the current table, which has been deleted.

---

sql/as_of_test.go, line 149 [r5] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Add a test for SELECT now() AS OF SYSTEM TIME '...'. (It returns the "as of" time and not the real time, right?)

It would be nice to support expressions like AS OF SYSTEM TIME now() - interval '1h' but it makes my head hurt to think about now() evaluating differently in different parts of the same statement.

That would return the real time, not the as of time, which is currently by design. I think that's correct because the syntax implies that we are querying a table at a specific time, not setting the whole statement to a time. If the syntax were something like `BEGIN READONLY AS OF `, then I think the time functions should return the as of time. Agree?

---

Comments from Reviewable

[knz]

Review status: 3 of 10 files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/as_of_test.go, line 56 [r5] (raw file):

Previously, mjibson (Matt Jibson) wrote…

No, we can't support placeholders. The query needs to be type-checked based on the table descriptor at the "as of" time. Type check is performed during the prepare phase to infer the placeholder types, and thus needs access to a table descriptor. But since we don't know the time at which we need the table descriptor, the current table descriptor is leased and used. Thus, if you try to read a column in the past that has been deleted in the future (which is valid, and we have a test to do so), then that test will fail if it's using a placeholder because it looks for the column in the current table, which has been deleted.

Well this is not entirely true; you can invoke TypeCheck then Eval on the SYSTEM TIME expression separately; as long as there are no QualifiedNames in there there will be no error due to missing table schema. Ping me if you want details.

---

Comments from Reviewable

[tbg]

---

Reviewed 1 of 2 files at r3, 4 of 6 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/as_of_test.go, line 149 [r5] (raw file):

Previously, mjibson (Matt Jibson) wrote…

That would return the real time, not the as of time, which is currently by design. I think that's correct because the syntax implies that we are querying a table at a specific time, not setting the whole statement to a time. If the syntax were something like BEGIN READONLY AS OF <time>, then I think the time functions should return the as of time. Agree?

In any case, worth testing are those time functions which actually use the txn timestamp (cluster_logical_timestamp?), unless you already do so somewhere.

---

sql/executor.go, line 461 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

I did a bunch of tests and am not convinced about this. The deadline checking examines the Timestamp field of the proto. Since (as of this most recent revision) I am setting .Timestamp at the start of each retry, I do not see how it could ever trigger the deadline failure. Indeed, with the modifications to the test suggested above, commenting out the deadline setting in executor.go doesn't change the output of the test. I have left the deadline setting in, because I don't think it does any harm and could potentially find problems in the future. But I'm not sure it's doing anything explicitly useful right now.

That's a good point. Could you add a comment to line 465 below?

---

sql/executor.go, line 421 [r3] (raw file):

Previously, mjibson (Matt Jibson) wrote…

I'm wrong. I forgot that they blocked lower down. In that case I am not sure what could theoretically cause a retry. In the case of old intents, it still sits there and blocks. That behavior is ok for now, as a user can ctrl+c out if the want. I agree we should add some kind of error or flag the store to not spin or something for this case.

Toby may know what can cause a retry to occur.

Yes, I don't see an immediate reason for a retry to actually occur. Regardless, there should be code that handles them somewhat gracefully - we shouldn't try to second-guess the KV layer (whose behavior may change over time).

---

sql/executor.go, line 1174 [r5] (raw file):

}

// isAsOf analyzes a select statement to bypass the login in newPlan(),

What do you mean by login?

---

Comments from Reviewable

[maddyblue]

Review status: all files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/as_of_test.go, line 56 [r5] (raw file):

Previously, knz (kena) wrote…

Well this is not entirely true; you can invoke TypeCheck then Eval on the SYSTEM TIME expression separately; as long as there are no QualifiedNames in there there will be no error due to missing table schema. Ping me if you want details.

What about this:

select a+$1 from t as of system time $2

It's impossible to know the type of a (and thus $1) until we know the value of $2 because a could have been dropped and added as a different type. If the system time placeholder is the only placeholder, then yes, we could delay type checking until execution time. But for now I'm going to leave it off.

---

sql/as_of_test.go, line 149 [r5] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

In any case, worth testing are those time functions which actually use the txn timestamp (cluster_logical_timestamp?), unless you already do so somewhere.

Test them for what, though? AS OF SYSTEM TIME only takes a string: you can't do `AS OF SYSTEM TIME cluster_logical_timestamp()`. Not sure what you mean here.

---

sql/executor.go, line 461 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

That's a good point. Could you add a comment to line 465 below?

Done.

---

sql/executor.go, line 1174 [r5] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

What do you mean by login?

typo. should be logic. fixed.

---

Comments from Reviewable

[bdarnell]

LGTM

---

Review status: 9 of 10 files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/as_of_test.go, line 56 [r5] (raw file):

Previously, mjibson (Matt Jibson) wrote…

What about this:

select a+$1 from t as of system time $2

It's impossible to know the type of a (and thus $1) until we know the value of $2 because a could have been dropped and added as a different type. If the system time placeholder is the only placeholder, then yes, we could delay type checking until execution time. But for now I'm going to leave it off.

OK, I'm fine pushing off placeholder support here.

---

sql/as_of_test.go, line 149 [r5] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Test them for what, though? AS OF SYSTEM TIME only takes a string: you can't do AS OF SYSTEM TIME cluster_logical_timestamp(). Not sure what you mean here.

I misremembered which time functions use which time source. I was suggesting that we have tests of `SELECT time_func() AS OF SYSTEM TIME 'T'` and verify that they do or do not use the specified time. (just to document and specify the behavior we expect here).

What about non-literal expressions in the AS OF SYSTEM TIME clause? It would be nice to support them, but only if it's easy.

---

Comments from Reviewable

[maddyblue]

Review status: 8 of 10 files reviewed at latest revision, 10 unresolved discussions, some commit checks pending.

---

sql/as_of_test.go, line 149 [r5] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I misremembered which time functions use which time source. I was suggesting that we have tests of SELECT time_func() AS OF SYSTEM TIME 'T' and verify that they do or do not use the specified time. (just to document and specify the behavior we expect here).

What about non-literal expressions in the AS OF SYSTEM TIME clause? It would be nice to support them, but only if it's easy.

That is difficult because supporting general expressions (as long as they are of type string) require a full evaluation context to correctly run. For example, the `now()` function asserts (with a panic if it fails) that its return value is not the zero timestamp. But since this expression is being evaluated way high up in the executor, there's not a well-populated eval context yet, and so now() and the other time functions will panic because their timestamps are still the zero timestamp. Thus, we support only string literals for now because we know that they don't assume anything about the eval context.

Added tests for the time functions, though. Will merge once the circle ci tests pass.

---

Comments from Reviewable

[tbg]

Reviewed 1 of 1 files at r6, 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

---

Comments from Reviewable