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.

Sign up for GitHub

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

Jump to bottom

'AS OF SYSTEM TIME' Can Read Incompatible Data #106764

Open
jeffswenson opened this issue Jul 13, 2023 · 0 comments
Open

'AS OF SYSTEM TIME' Can Read Incompatible Data #106764

jeffswenson opened this issue Jul 13, 2023 · 0 comments
Labels
A-cluster-upgrades A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@jeffswenson
Copy link
Collaborator

jeffswenson commented Jul 13, 2023 •
edited
Loading

Describe the problem

The 'AS OF SYSTEM TIME' clause allows users to pick the logical timestamp to execute a query at. When running 'AS OF SYSTEM TIME' queries, sql uses descriptors from the provided timestamp. This could cause problems when combined with serial major version upgrades and migrations.

Consider the 22.1, 22.2, and 23.1 upgrade chain. 23.1 is not guaranteed to be compatible with 22.1 descriptors.

Example:

t0: cluster is running 22.1
t1: binary is upgraded to 22.2
t2: 22.2 migrations are run
t3: binary is upgraded to 23.1
t4: 23.1 migrations are run
t5: A query is run with a system time of t0.

In this example the 23.1 query would try to interpret the descriptors from t0, which may cause the query to return incorrect results.

This problem could impact any sub system that depends on version gates and migrations. See #106762 for an example that is not dependent on descriptors.

The easiest way to "fix" this problem would be to reject AS OF SYSTEM TIME queries that attempt to read logical timestamps that were written by a binary older than clusterversion.BinaryMinSupportedVersionKey.

Jira issue: CRDB-29708
@jeffswenson jeffswenson added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-execution Relating to SQL execution. A-cluster-upgrades labels Jul 13, 2023
adityamaru added a commit to adityamaru/cockroach that referenced this issue Jul 25, 2023
@adityamaru
sql,jobs: short-term fix for UndefinedColumn job_type error
ac55715 
In cockroachdb#106762 we noticed that if
a query is executed with an AS OF SYSTEM TIME clause that picks a transaction
timestamp before the job_type migration, then parts of the jobs
infrastructure will attempt to query the job_type column even though it
doesn't exist at the transaction's timestamp.

As a short term fix, when we encounter an `UndefinedColumn` error for
the `job_type` column in `crdb_internal.jobs` we
generate a synthetic retryable error so that the txn is pushed to a
higher timestamp at which the upgrade will have completed and the
`job_type` column will be visible. The longer term fix is being tracked
in cockroachdb#106764.

We are intentionally approaching this issue with a whack-a-mole
approach to stabilize the tests the are running into this issue. We
think time is better spent designing and investing in the longer term
solution that will be tracked in cockroachdb#106764.

Fixes: cockroachdb#107169
Informs: cockroachdb#106762
Release note: None
@adityamaru adityamaru mentioned this issue Jul 25, 2023
Merged
craig bot pushed a commit that referenced this issue Jul 26, 2023
@adityamaru @mgartner
Merge #107570 #107641
1d3c11e 
107570: sql,jobs: short-term fix for `UndefinedColumn` job_type error r=HonoreDB a=adityamaru

In #106762 we noticed that if a query is executed with an AS OF SYSTEM TIME clause that picks a transaction timestamp before the job_type migration, then parts of the jobs infrastructure will attempt to query the job_type column even though it doesn't exist at the transaction's timestamp.

As a short term fix, when we encounter an `UndefinedColumn` error for the `job_type` column in `crdb_internal.jobs` we
generate a synthetic retryable error so that the txn is pushed to a higher timestamp at which the upgrade will have completed and the `job_type` column will be visible. The longer term fix is being tracked in #106764.

We are intentionally approaching this issue with a whack-a-mole approach to stabilize the tests the are running into this issue. We think time is better spent designing and investing in the longer term solution that will be tracked in #106764.

Fixes: #107169
Informs: #106762
Release note: None

107641: sql: disallow aggregate functions in ORDER BY in DELETE and UPDATE r=mgartner a=mgartner

This commit disallows aggregate functions in the context of an
`ORDER BY` clause in a `DELETE` or `UPDATE` statement. An aggregate
function in an `ORDER BY` would require a `GROUP BY` clause to group
non-aggregate columns. A `GROUP BY` is not allowed in `DELETE` or
`UPDATE` statements as it's not obvious how grouping in these statements
would behave. So we simply disallow aggregates in `ORDER BY` instead.

Fixes #107634

Release note (bug fix): A bug has been fixed that caused internal errors
when using an aggregate function in an `ORDER BY` clause of a `DELETE`
or `UPDATE` statement. Aggregate functions are no longer allowed in
these contexts. The bug has been present since at least version 20.2.


Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
@adityamaru adityamaru mentioned this issue Jul 31, 2023
Closed
adityamaru added a commit to adityamaru/cockroach that referenced this issue Aug 8, 2023
@adityamaru
jobs: fix mixed-version jobs flake
79cc2ea 
Similar to cockroachdb#107570
this is a short term fix for when an a query is executed with an AS OF SYSTEM TIME
picks a transaction timestamp before the job_info migration has run.
In which case parts of the jobs infrastructure will attempt to query
the job_info column even though it doesn't exist at the transaction's timestamp.

As a short term fix, when we encounter an UndefinedObject error for the job_info table
we generate a synthetic retryable error so that the txn is pushed to a higher timestamp
at which the upgrade will have completed and the job_info table will be visible.
The longer term fix is being tracked in cockroachdb#106764.

On master I can no longer reproduce the failure in cockroachdb#105032 but
on 23.1 with this change I can successfully run 30 iterations of the test
on a seed (-8690666577594439584) which previously saw occurrences
of this flake.

Fixes: cockroachdb#103239
Fixes: cockroachdb#105032

Release note: None
@adityamaru adityamaru mentioned this issue Aug 8, 2023
Merged
craig bot pushed a commit that referenced this issue Aug 12, 2023
@adityamaru @erikgrinaker
Merge #108357 #108583
9184ffb 
108357: jobs: fix mixed-version jobs flake r=knz a=adityamaru

Similar to #107570 this is a short term fix for when an a query is executed with an AS OF SYSTEM TIME picks a transaction timestamp before the job_info migration has run. In which case parts of the jobs infrastructure will attempt to query the job_info column even though it doesn't exist at the transaction's timestamp.

As a short term fix, when we encounter an UndefinedObject error for the job_info table we generate a synthetic retryable error so that the txn is pushed to a higher timestamp at which the upgrade will have completed and the job_info table will be visible. The longer term fix is being tracked in #106764.

On master I can no longer reproduce the failure in #105032 but on 23.1 with this change I can successfully run 30 iterations of the test on a seed (-8690666577594439584) which previously saw occurrences of this flake.

Fixes: #103239
Fixes: #105032

Release note: None

108583: rangefeed: deflake `TestBudgetReleaseOnOneStreamError` r=erikgrinaker a=erikgrinaker

The test could fail with `REASON_SLOW_CONSUMER` if the registration goroutine did not drain the queue in time (1 ms). Increase the timeout.

Resolves #108555.

Epic: none
Release note: None

Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
@blathers-crl blathers-crl bot mentioned this issue Aug 12, 2023
Closed
blathers-crl bot pushed a commit that referenced this issue Aug 12, 2023
@adityamaru
jobs: fix mixed-version jobs flake
36e38cd 
Similar to #107570
this is a short term fix for when an a query is executed with an AS OF SYSTEM TIME
picks a transaction timestamp before the job_info migration has run.
In which case parts of the jobs infrastructure will attempt to query
the job_info column even though it doesn't exist at the transaction's timestamp.

As a short term fix, when we encounter an UndefinedObject error for the job_info table
we generate a synthetic retryable error so that the txn is pushed to a higher timestamp
at which the upgrade will have completed and the job_info table will be visible.
The longer term fix is being tracked in #106764.

On master I can no longer reproduce the failure in #105032 but
on 23.1 with this change I can successfully run 30 iterations of the test
on a seed (-8690666577594439584) which previously saw occurrences
of this flake.

Fixes: #103239
Fixes: #105032

Release note: None
adityamaru added a commit that referenced this issue Aug 18, 2023
@adityamaru
sql,jobs: short-term fix for UndefinedColumn job_type error
63a71dc 
In #106762 we noticed that if
a query is executed with an AS OF SYSTEM TIME clause that picks a transaction
timestamp before the job_type migration, then parts of the jobs
infrastructure will attempt to query the job_type column even though it
doesn't exist at the transaction's timestamp.

As a short term fix, when we encounter an `UndefinedColumn` error for
the `job_type` column in `crdb_internal.jobs` we
generate a synthetic retryable error so that the txn is pushed to a
higher timestamp at which the upgrade will have completed and the
`job_type` column will be visible. The longer term fix is being tracked
in #106764.

We are intentionally approaching this issue with a whack-a-mole
approach to stabilize the tests the are running into this issue. We
think time is better spent designing and investing in the longer term
solution that will be tracked in #106764.

Fixes: #107169
Informs: #106762
Release note: None
adityamaru added a commit that referenced this issue Aug 18, 2023
@adityamaru
jobs: fix mixed-version jobs flake
c46c488 
Similar to #107570
this is a short term fix for when an a query is executed with an AS OF SYSTEM TIME
picks a transaction timestamp before the job_info migration has run.
In which case parts of the jobs infrastructure will attempt to query
the job_info column even though it doesn't exist at the transaction's timestamp.

As a short term fix, when we encounter an UndefinedObject error for the job_info table
we generate a synthetic retryable error so that the txn is pushed to a higher timestamp
at which the upgrade will have completed and the job_info table will be visible.
The longer term fix is being tracked in #106764.

On master I can no longer reproduce the failure in #105032 but
on 23.1 with this change I can successfully run 30 iterations of the test
on a seed (-8690666577594439584) which previously saw occurrences
of this flake.

Fixes: #103239
Fixes: #105032

Release note: None
@adityamaru adityamaru mentioned this issue Sep 20, 2023
Closed
@healthy-pod healthy-pod mentioned this issue Oct 6, 2023
Closed
@dt dt mentioned this issue Nov 3, 2023
Closed
@adityamaru adityamaru mentioned this issue Dec 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-cluster-upgrades A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@jeffswenson