Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent AS OF SYSTEM TIME future value behavior #91021

Closed
ecwall opened this issue Oct 31, 2022 · 2 comments · Fixed by #93146
Closed

Inconsistent AS OF SYSTEM TIME future value behavior #91021

ecwall opened this issue Oct 31, 2022 · 2 comments · Fixed by #93146
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ecwall
Copy link
Contributor

ecwall commented Oct 31, 2022

Actual behavior fails randomly with small AS OF SYSTEM TIME intervals in the future:

[email protected]:26257/movr> CREATE TABLE t();
CREATE TABLE


Time: 3ms total (execution 3ms / network 0ms)

[email protected]:26257/movr> SELECT * FROM t AS OF SYSTEM TIME interval '20 microsecond';
--
(0 rows)


Time: 1ms total (execution 1ms / network 0ms)

[email protected]:26257/movr> SELECT * FROM t AS OF SYSTEM TIME interval '20 microsecond';
ERROR: AS OF SYSTEM TIME: cannot specify timestamp in the future (1667249155.165540000,0 > 1667249155.165537000,0)

Expected behavior: should always fail with any interval > 0.

Happens in master as of 3dd5e68701273a51c076ce4d6835575e9991d8b0.

Jira issue: CRDB-21077

Epic CRDB-17785

@ecwall ecwall added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Oct 31, 2022
@ajwerner
Copy link
Contributor

heh this one is fun :) It's succeeding sometimes because by the time it calls clock.Now 20us have passed.

@ajwerner
Copy link
Contributor

It's at least easy to fix. Maybe something like:

--- a/pkg/sql/sem/asof/as_of.go
+++ b/pkg/sql/sem/asof/as_of.go
@@ -235,7 +235,7 @@ func DatumToHLC(
                }
                // Attempt to parse as an interval.
                if iv, err := tree.ParseDInterval(evalCtx.GetIntervalStyle(), s); err == nil {
-                       if (iv.Duration == duration.Duration{}) {
+                       if iv.Duration.Compare(duration.Duration{}) <= 0 {
                                convErr = errors.Errorf("interval value %v too small, absolute value must be >= %v", d, time.Microsecond)
                        }
                        ts.WallTime = duration.Add(stmtTimestamp, iv.Duration).UnixNano()
@@ -252,6 +252,9 @@ func DatumToHLC(
        case *tree.DDecimal:
                ts, convErr = hlc.DecimalToHLC(&d.Decimal)
        case *tree.DInterval:
+               if d.Duration.Compare(duration.Duration{}) <= 0 {
+                       convErr = errors.Errorf("interval value %v too small, absolute value must be >= %v", d, time.Microsecond)
+               }
                ts.WallTime = duration.Add(stmtTimestamp, d.Duration).UnixNano()

@exalate-issue-sync exalate-issue-sync bot assigned ZhouXing19 and unassigned e-mbrown Nov 14, 2022
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Dec 7, 2022
fixes cockroachdb#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 nanosecond.

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
nanosecond.

link epic CRDB-17785

Release note (bug fix): add restriction for duration value for AS OF SYSTEM TIME
and SPLIT statement.
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Dec 7, 2022
fixes cockroachdb#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 nanosecond.

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
nanosecond.

link epic CRDB-17785

Release note (bug fix): add restriction for duration value for AS OF SYSTEM TIME
and SPLIT statement.
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Dec 7, 2022
  fixes cockroachdb#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 nanosecond.

  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
  nanosecond.

  link epic CRDB-17785

  Release note (bug fix): add restriction for duration value for AS OF SYSTEM TIME
  and SPLIT statement.
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Dec 7, 2022
  fixes cockroachdb#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 nanosecond.

  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
  nanosecond.

  link epic CRDB-17785

  Release note (bug fix): add restriction for duration value for AS OF SYSTEM TIME
  and SPLIT statement.
craig bot pushed a commit that referenced this issue Dec 9, 2022
90649: sql: support indexes in regclass cast and pg_table_is_visible r=ajwerner a=rafiss

fixes #88097

### sql: remove deprecated function from DatabaseCatalog

The ParseQualifiedTableName function was deprecated and unused.

### sql: support casts from index name to regclass

Release note (sql change): Casts from index name to regclass are now
supported. Previously, only table names could be cast to regclass.

### sql: add index on pg_namespace(oid)

This will allow us to efficiently join to this table.

### sql: fix pg_table_is_visible to handle indexes

This uses the internal executor now to do the introspection using
pg_class where everything can be looked up all at once.

There's an increase in number of round trips, but it's a constant
factor.

Release note (bug fix): Fixed the pg_table_is_visible builtin function
so it correctly reports visibility of indexes based on the current
search_path.

91965: allocator: add support for store pool liveness overrides r=AlexTalks a=AlexTalks

While previously the allocator only evaluated using liveness obtained
from gossip, this change introduces a new `OverrideStorePool` struct
which can be used to override the liveness of a node for the purposes of
evaluating allocator actions and targets.  This `OverrideStorePool` is
backed by an existing actual `StorePool`, which retains the majority of
its logic.

Depends on #91461.

Part of #91570.

Release note: None

93146: sql/sem: add check for interval for asof.DatumToHLC() r=rafiss a=ZhouXing19

Currently, if the given interval for `AS OF SYSTEM TIME interval` is a small postive duration, the query can 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 greater than a nanosecond.

fixes #91021
link epic CRDB-17785

Release note (bug fix): add restriction for duration value for AS OF SYSTEM TIME statement.

93340: roachtest: add flaky test to activerecord ignore list r=ZhouXing19 a=andyyang890

Fixes #93189

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
@craig craig bot closed this as completed in 14c89e1 Dec 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants