sql: apply PCR AOST time stamp for authentication flow #135929
+153
−78
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, when using PCR reader catalog its was possible to hit "PCR reader timestamp has moved forward" when attempting to execute workloads. This was because internal executors are exempt from the AOST timestamp. To address this, the authentication code paths are updated to set fixed timestamps, when connecting to a PCR reader catalog. Additionally, additional tests are added for validating opening new connections and preparing queries to confirm this problem is fixed. Fixes: cockroachdb#135829 Release note: None
|
This change is |
msbutler
reviewed
Nov 21, 2024
There was a problem hiding this comment.
thanks for the patch!! I'll try running my roachtest on top of this. I'll let someone from foundations approve this.
One other thought: I wonder if there's a more general way to find other internal executor read queries that require a fixed timestamp. Or maybe this points to a larger design problem: perhaps no system tables should be externalized. cc @dt
| @@ -322,6 +328,16 @@ func TestReaderCatalogTSAdvance(t *testing.T) { | |||
| } else { | |||
| require.NotEqualValues(t, srcRes, destRes) | |||
| } | |||
|
|
|||
| // Sanity: Execute the same query as prepared statement inside the reader | |||
There was a problem hiding this comment.
to confirm, were these unit tests with prepared stmts failing before your patch to the source code?
| @@ -432,7 +473,7 @@ func TestReaderCatalogTSAdvance(t *testing.T) { | |||
| // with PCR even if a long running txn is running. | |||
| func TestReaderCatalogTSAdvanceWithLongTxn(t *testing.T) { | |||
| defer leaktest.AfterTest(t)() | |||
| skip.UnderDuress(t) | |||
| // skip.UnderDuress(t) | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when using PCR reader catalog its was possible to hit "PCR reader timestamp has moved forward" when attempting to execute workloads. This was because internal executors are exempt from the AOST timestamp. To address this, the authentication code paths are updated to set fixed timestamps, when connecting to a PCR reader catalog. Additionally, additional tests are added for validating opening new connections and preparing queries to confirm this problem is fixed.
Fixes: #135829
Release note: None