Skip to content

Commit

Permalink
sql/sem: add check for interval for asof.DatumToHLC()
Browse files Browse the repository at this point in the history
  fixes #91021

  Currently, if the given interval for `AS OF SYSTEM TIME interval` is a small
  postive duration, the query will incorrectly pass. It's because when we call
  `clock.Now()`, it has passed the threashold ('statement timestamp + duration').

  Now we add a check for the duration value. It has to be negative, with the
  absolute value >= a microsecond.

  Similarly, when the logic is used under the SPLIT stmt, the interval's value has
  to be positive (for the experation duration), with an absolute value >= a
  microsecond.

  link epic CRDB-17785

  Release note (bug fix): add restriction for duration value for AS OF SYSTEM TIME
  and SPLIT statement.
  • Loading branch information
ZhouXing19 committed Dec 9, 2022
1 parent 916ca31 commit 14c89e1
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 5 deletions.
5 changes: 4 additions & 1 deletion pkg/sql/logictest/testdata/logic_test/as_of
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,12 @@ 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 cannot specify timestamp in the future
statement error pq: AS OF SYSTEM TIME: interval value '10s' too large, AS OF interval must be <= -1µs
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
SELECT * FROM t AS OF SYSTEM TIME interval '1 microsecond'

# Verify that the TxnTimestamp used to generate now() and current_timestamp() is
# set to the historical timestamp.

Expand Down
29 changes: 27 additions & 2 deletions pkg/sql/sem/asof/as_of.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,16 +199,30 @@ func Eval(
}

stmtTimestamp := evalCtx.GetStmtTimestamp()
ret.Timestamp, err = DatumToHLC(evalCtx, stmtTimestamp, d)
ret.Timestamp, err = DatumToHLC(evalCtx, stmtTimestamp, d, AsOf)
if err != nil {
return eval.AsOfSystemTime{}, errors.Wrap(err, "AS OF SYSTEM TIME")
}
return ret, nil
}

// DatumToHLCUsage specifies which statement DatumToHLC() is used for.
type DatumToHLCUsage int64

const (
// AsOf is when the DatumToHLC() is used for an AS OF SYSTEM TIME statement.
// In this case, if the interval is not synthetic, its value has to be negative
// and last longer than a nanosecond.
AsOf DatumToHLCUsage = iota
// Split is when the DatumToHLC() is used for a SPLIT statement.
// In this case, if the interval is not synthetic, its value has to be positive
// and last longer than a nanosecond.
Split
)

// DatumToHLC performs the conversion from a Datum to an HLC timestamp.
func DatumToHLC(
evalCtx *eval.Context, stmtTimestamp time.Time, d tree.Datum,
evalCtx *eval.Context, stmtTimestamp time.Time, d tree.Datum, usage DatumToHLCUsage,
) (hlc.Timestamp, error) {
ts := hlc.Timestamp{}
var convErr error
Expand Down Expand Up @@ -237,6 +251,12 @@ 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()
ts.Synthetic = syn
Expand All @@ -252,6 +272,11 @@ 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) {
convErr = errors.Errorf("interval value %v too small, SPLIT interval must be >= %v", d, time.Microsecond)
}
ts.WallTime = duration.Add(stmtTimestamp, d.Duration).UnixNano()
default:
convErr = errors.WithSafeDetails(
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/split.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func parseExpirationTime(
return hlc.MaxTimestamp, nil
}
stmtTimestamp := evalCtx.GetStmtTimestamp()
ts, err := asof.DatumToHLC(evalCtx, stmtTimestamp, d)
ts, err := asof.DatumToHLC(evalCtx, stmtTimestamp, d, asof.Split)
if err != nil {
return ts, errors.Wrap(err, "SPLIT AT")
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/split_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func TestSplitAt(t *testing.T) {
},
{
in: "ALTER TABLE d.i SPLIT AT VALUES (17) WITH EXPIRATION '-1 day'::interval",
error: "SPLIT AT: expiration time should be greater than or equal to current time",
error: "SPLIT AT: interval value '-1 days' too small, SPLIT interval must be >= 1µs",
},
{
in: "ALTER TABLE d.i SPLIT AT VALUES (17) WITH EXPIRATION '0.1us'",
Expand Down

0 comments on commit 14c89e1

Please sign in to comment.