From 34dc5ae37841b069d202a369f4d4c5783af001e2 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Wed, 3 Mar 2021 21:49:04 -0500 Subject: [PATCH] sql: support AOST queries with synthetic timestamps This commit updates the handling of AS OF SYSTEM TIME clauses to accept future-time synthetic timestamps. This functionality is needed when performing certain kinds of schema changes on GLOBAL tables, as the operating transaction will get its commit timestamp bumped into the future and will then try to call into `CountLeases` from `CheckTwoVersionInvariant` with the commit timestamp. We're already protected from users shooting themselves in the foot too badly with this, as we only allow timestamps to lead present time by a few seconds before they are rejected by KV. Release justification: Needed for new functionality --- pkg/sql/as_of_test.go | 12 +++++++++++- pkg/sql/catalog/lease/lease.go | 2 +- pkg/sql/exec_util.go | 2 +- pkg/sql/sem/tree/BUILD.bazel | 1 + pkg/sql/sem/tree/as_of.go | 12 ++++++++++++ pkg/util/hlc/timestamp.go | 6 +++++- pkg/util/hlc/timestamp_test.go | 3 +++ 7 files changed, 34 insertions(+), 4 deletions(-) diff --git a/pkg/sql/as_of_test.go b/pkg/sql/as_of_test.go index bf7fd5b8b02f..0941401ba4bc 100644 --- a/pkg/sql/as_of_test.go +++ b/pkg/sql/as_of_test.go @@ -122,11 +122,21 @@ func TestAsOfTime(t *testing.T) { } }) - // Future queries shouldn't work. + // 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") { + 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 { + t.Fatal(err) + } + // Verify queries with positive scale work properly. if _, err := db.Query("SELECT a FROM d.t AS OF SYSTEM TIME 1e1"); !testutils.IsError(err, `pq: relation "d.t" does not exist`) { t.Fatal(err) diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index db40f7af8f1b..985a9e0155fb 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -400,7 +400,7 @@ func CountLeases( ) } - stmt := fmt.Sprintf(`SELECT count(1) FROM system.public.lease AS OF SYSTEM TIME %s WHERE `, + stmt := fmt.Sprintf(`SELECT count(1) FROM system.public.lease AS OF SYSTEM TIME '%s' WHERE `, at.AsOfSystemTime()) + strings.Join(whereClauses, " OR ") values, err := executor.QueryRowEx( diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 2041b214ad47..e7c93de2741c 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -1222,7 +1222,7 @@ func (p *planner) EvalAsOfTimestamp( if err != nil { return hlc.Timestamp{}, err } - if now := p.execCfg.Clock.Now(); now.Less(ts) { + if now := p.execCfg.Clock.Now(); now.Less(ts) && !ts.Synthetic { return hlc.Timestamp{}, errors.Errorf( "AS OF SYSTEM TIME: cannot specify timestamp in the future (%s > %s)", ts, now) } diff --git a/pkg/sql/sem/tree/BUILD.bazel b/pkg/sql/sem/tree/BUILD.bazel index 06cd229f69ac..c2bd52d625af 100644 --- a/pkg/sql/sem/tree/BUILD.bazel +++ b/pkg/sql/sem/tree/BUILD.bazel @@ -105,6 +105,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/base", + "//pkg/clusterversion", "//pkg/geo", "//pkg/geo/geopb", "//pkg/keys", diff --git a/pkg/sql/sem/tree/as_of.go b/pkg/sql/sem/tree/as_of.go index 6c2b6bb0550e..453b0ed8368f 100644 --- a/pkg/sql/sem/tree/as_of.go +++ b/pkg/sql/sem/tree/as_of.go @@ -17,6 +17,7 @@ import ( "time" "github.com/cockroachdb/apd/v2" + "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" @@ -106,14 +107,24 @@ func DatumToHLC(evalCtx *EvalContext, stmtTimestamp time.Time, d Datum) (hlc.Tim switch d := d.(type) { case *DString: s := string(*d) + // Parse synthetic flag. + syn := false + if strings.HasSuffix(s, "?") && evalCtx.Settings.Version.IsActive(evalCtx.Context, clusterversion.PriorReadSummaries) { + // NOTE: we don't parse this in mixed-version clusters because v20.2 + // nodes will not know how to handle synthetic timestamps. + s = s[:len(s)-1] + syn = true + } // Attempt to parse as timestamp. if dt, _, err := ParseDTimestamp(evalCtx, s, time.Nanosecond); err == nil { ts.WallTime = dt.Time.UnixNano() + ts.Synthetic = syn break } // Attempt to parse as a decimal. if dec, _, err := apd.NewFromString(s); err == nil { ts, convErr = DecimalToHLC(dec) + ts.Synthetic = syn break } // Attempt to parse as an interval. @@ -122,6 +133,7 @@ func DatumToHLC(evalCtx *EvalContext, stmtTimestamp time.Time, d Datum) (hlc.Tim convErr = errors.Errorf("interval value %v too small, absolute value must be >= %v", d, time.Microsecond) } ts.WallTime = duration.Add(stmtTimestamp, iv.Duration).UnixNano() + ts.Synthetic = syn break } convErr = errors.Errorf("value is neither timestamp, decimal, nor interval") diff --git a/pkg/util/hlc/timestamp.go b/pkg/util/hlc/timestamp.go index af127a9c2457..550cab61b671 100644 --- a/pkg/util/hlc/timestamp.go +++ b/pkg/util/hlc/timestamp.go @@ -168,7 +168,11 @@ func ParseTimestamp(str string) (_ Timestamp, err error) { // AsOfSystemTime returns a string to be used in an AS OF SYSTEM TIME query. func (t Timestamp) AsOfSystemTime() string { - return fmt.Sprintf("%d.%010d", t.WallTime, t.Logical) + syn := "" + if t.Synthetic { + syn = "?" + } + return fmt.Sprintf("%d.%010d%s", t.WallTime, t.Logical, syn) } // IsEmpty retruns true if t is an empty Timestamp. diff --git a/pkg/util/hlc/timestamp_test.go b/pkg/util/hlc/timestamp_test.go index 4e4433874f58..8efc12f6a9aa 100644 --- a/pkg/util/hlc/timestamp_test.go +++ b/pkg/util/hlc/timestamp_test.go @@ -222,6 +222,9 @@ func TestAsOfSystemTime(t *testing.T) { {makeTS(145, 0), "145.0000000000"}, {makeTS(145, 123), "145.0000000123"}, {makeTS(145, 1123456789), "145.1123456789"}, + {makeSynTS(145, 0), "145.0000000000?"}, + {makeSynTS(145, 123), "145.0000000123?"}, + {makeSynTS(145, 1123456789), "145.1123456789?"}, } for _, c := range testCases { assert.Equal(t, c.exp, c.ts.AsOfSystemTime())