From f719e8566cf78a3d86e27fe169730cf2abd6a24e Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Tue, 26 Dec 2023 21:18:44 -0500 Subject: [PATCH] sql: permit non-synthetic future time AOST queries Informs #101938. This commit removes the restriction that future-time AOST queries must use "synthetic" timestamps. This restriction was introduced back when it was unsafe for requests to operate at future timestamps unless they were marked as synthetic. This is no longer the case. It is important that we allow this, because future-time queries can issue subqueries with AOST timestamps (e.g. for SQL leasing with the "count-leases" internal executor use). Requests that are too far in the future will still be rejected during range lease checks. For details, see `Replica.checkRequestTimeRLocked`. Release note: None --- pkg/ccl/changefeedccl/changefeed_test.go | 2 +- pkg/sql/as_of_test.go | 11 +++-------- pkg/sql/exec_util.go | 5 ----- pkg/sql/logictest/testdata/logic_test/as_of | 6 ++++-- pkg/sql/pgwire/testdata/pgtest/bind_and_resolve | 2 +- pkg/sql/sem/asof/as_of.go | 8 +------- pkg/sql/tests/rsg_test.go | 1 - pkg/testutils/sqlutils/sql_runner.go | 2 +- 8 files changed, 11 insertions(+), 26 deletions(-) diff --git a/pkg/ccl/changefeedccl/changefeed_test.go b/pkg/ccl/changefeedccl/changefeed_test.go index 8287e1b0941a..bbb146db773d 100644 --- a/pkg/ccl/changefeedccl/changefeed_test.go +++ b/pkg/ccl/changefeedccl/changefeed_test.go @@ -5021,7 +5021,7 @@ func TestChangefeedErrors(t *testing.T) { ) sqlDB.ExpectErrWithTimeout( - t, `cannot specify timestamp in the future`, + t, `request timestamp .* too far in future`, `EXPERIMENTAL CHANGEFEED FOR foo WITH cursor=$1`, timeutil.Now().Add(time.Hour), ) diff --git a/pkg/sql/as_of_test.go b/pkg/sql/as_of_test.go index 101ab6a28e91..fa4e58cd10a1 100644 --- a/pkg/sql/as_of_test.go +++ b/pkg/sql/as_of_test.go @@ -132,18 +132,13 @@ func TestAsOfTime(t *testing.T) { t.Fatal(err) } - // Future queries shouldn't work if not marked as synthetic. - if err := db.QueryRow("SELECT a FROM d.t AS OF SYSTEM TIME '2200-01-01'").Scan(&i); !testutils.IsError(err, "pq: AS OF SYSTEM TIME: cannot specify timestamp in the future") { - t.Fatal(err) - } - // Future queries shouldn't work if too far in the future. - if err := db.QueryRow("SELECT a FROM d.t AS OF SYSTEM TIME '+10h?'").Scan(&i); !testutils.IsError(err, "pq: request timestamp .* too far in future") { + if err := db.QueryRow("SELECT a FROM d.t AS OF SYSTEM TIME '+10h'").Scan(&i); !testutils.IsError(err, "pq: request timestamp .* too far in future") { t.Fatal(err) } - // Future queries work if marked as synthetic and only slightly in future. - if err := db.QueryRow("SELECT a FROM d.t AS OF SYSTEM TIME '+10ms?'").Scan(&i); err != nil { + // Future queries work if only slightly in the future. + if err := db.QueryRow("SELECT a FROM d.t AS OF SYSTEM TIME '+10ms'").Scan(&i); err != nil { t.Fatal(err) } diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 6f2a6d7db75b..ab47fbd17533 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -2057,11 +2057,6 @@ func (p *planner) EvalAsOfTimestamp( if err != nil { return eval.AsOfSystemTime{}, err } - ts := asOf.Timestamp - if now := p.execCfg.Clock.Now(); now.Less(ts) && !ts.Synthetic { - return eval.AsOfSystemTime{}, errors.Errorf( - "AS OF SYSTEM TIME: cannot specify timestamp in the future (%s > %s)", ts, now) - } return asOf, nil } diff --git a/pkg/sql/logictest/testdata/logic_test/as_of b/pkg/sql/logictest/testdata/logic_test/as_of index a794436c5d97..3b18617bce1f 100644 --- a/pkg/sql/logictest/testdata/logic_test/as_of +++ b/pkg/sql/logictest/testdata/logic_test/as_of @@ -50,11 +50,13 @@ SELECT * FROM t AS OF SYSTEM TIME follower_read_timestamp('boom') statement error pq: AS OF SYSTEM TIME: only constant expressions, with_min_timestamp, with_max_staleness, or follower_read_timestamp are allowed SELECT * FROM t AS OF SYSTEM TIME now() -statement error pq: AS OF SYSTEM TIME: interval value '10s' too large, AS OF interval must be <= -1µs +statement error pq: request timestamp .* too far in future SELECT * FROM t AS OF SYSTEM TIME '10s' -statement error pq: AS OF SYSTEM TIME: interval value '00:00:00.000001' too large, AS OF interval must be <= -1µs +query I SELECT * FROM t AS OF SYSTEM TIME interval '1 microsecond' +---- +2 # Verify that the TxnTimestamp used to generate now() and current_timestamp() is # set to the historical timestamp. diff --git a/pkg/sql/pgwire/testdata/pgtest/bind_and_resolve b/pkg/sql/pgwire/testdata/pgtest/bind_and_resolve index fd1b86f6bb04..fb2ba3f6b633 100644 --- a/pkg/sql/pgwire/testdata/pgtest/bind_and_resolve +++ b/pkg/sql/pgwire/testdata/pgtest/bind_and_resolve @@ -40,7 +40,7 @@ ReadyForQuery # This is crdb_only because Postgres does not support AS OF SYSTEM TIME. send crdb_only -Query {"String": "BEGIN AS OF SYSTEM TIME '1s'"} +Query {"String": "BEGIN AS OF SYSTEM TIME '0s'"} Sync ---- diff --git a/pkg/sql/sem/asof/as_of.go b/pkg/sql/sem/asof/as_of.go index baff941f6d93..799a5fc53572 100644 --- a/pkg/sql/sem/asof/as_of.go +++ b/pkg/sql/sem/asof/as_of.go @@ -266,11 +266,7 @@ func DatumToHLC( if iv, err := tree.ParseDInterval(evalCtx.GetIntervalStyle(), s); err == nil { if (iv.Duration == duration.Duration{}) { convErr = errors.Errorf("interval value %v too small, absolute value must be >= %v", d, time.Microsecond) - } else if (usage == AsOf && iv.Duration.Compare(duration.Duration{}) > 0 && !syn) { - convErr = errors.Errorf("interval value %v too large, AS OF interval must be <= -%v", d, time.Microsecond) } else if (usage == Split && iv.Duration.Compare(duration.Duration{}) < 0) { - // Do we need to consider if the timestamp is synthetic (see - // hlc.Timestamp.Synthetic), as for AS OF stmt? convErr = errors.Errorf("interval value %v too small, SPLIT AT interval must be >= %v", d, time.Microsecond) } ts.WallTime = duration.Add(stmtTimestamp, iv.Duration).UnixNano() @@ -287,9 +283,7 @@ func DatumToHLC( case *tree.DDecimal: ts, convErr = hlc.DecimalToHLC(&d.Decimal) case *tree.DInterval: - if (usage == AsOf && d.Duration.Compare(duration.Duration{}) > 0) { - convErr = errors.Errorf("interval value %v too large, AS OF interval must be <= -%v", d, time.Microsecond) - } else if (usage == Split && d.Duration.Compare(duration.Duration{}) < 0) { + if (usage == Split && d.Duration.Compare(duration.Duration{}) < 0) { convErr = errors.Errorf("interval value %v too small, SPLIT interval must be >= %v", d, time.Microsecond) } ts.WallTime = duration.Add(stmtTimestamp, d.Duration).UnixNano() diff --git a/pkg/sql/tests/rsg_test.go b/pkg/sql/tests/rsg_test.go index a3b0e19ac7c5..29defd3fe447 100644 --- a/pkg/sql/tests/rsg_test.go +++ b/pkg/sql/tests/rsg_test.go @@ -560,7 +560,6 @@ var ignoredErrorPatterns = []string{ "index .* in the middle of being added", "could not mark job .* as succeeded", "failed to read backup descriptor", - "AS OF SYSTEM TIME: cannot specify timestamp in the future", "AS OF SYSTEM TIME: timestamp before 1970-01-01T00:00:00Z is invalid", "BACKUP for requested time needs option 'revision_history'", "RESTORE timestamp: supplied backups do not cover requested time", diff --git a/pkg/testutils/sqlutils/sql_runner.go b/pkg/testutils/sqlutils/sql_runner.go index c6ee4d32b8e0..99658712abd1 100644 --- a/pkg/testutils/sqlutils/sql_runner.go +++ b/pkg/testutils/sqlutils/sql_runner.go @@ -229,7 +229,7 @@ func (sr *SQLRunner) ExpectErrSucceedsSoon( }) } -// ExpectErrWithTimeout wraps ExpectErr with a timeout.. +// ExpectErrWithTimeout wraps ExpectErr with a timeout. func (sr *SQLRunner) ExpectErrWithTimeout( t Fataler, errRE string, query string, args ...interface{}, ) {