staleread: Allow passing TSO in AS OF TIMESTAMP clause#66643
staleread: Allow passing TSO in AS OF TIMESTAMP clause#66643madhavramesh wants to merge 6 commits intopingcap:masterfrom
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Review Complete Findings: 1 issues |
ti-chi-bot
bot
added
do-not-merge/release-note-label-needed
|
Hi @madhavramesh. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @madhavramesh. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
madhavramesh
changed the title
madhavramesh
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAS OF TIMESTAMP parsing now tries to interpret the expression as a datetime first; if that fails it falls back to parsing a numeric TSO (uint64), validates the TSO's physical time, and returns an exact staleness TS. New unit and integration tests exercise datetime, compact-datetime, numeric TSO, and invalid cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/sessiontxn/staleread/util.go (1)
69-77: Consider handling integer Datum kinds for TSO values.The current implementation only parses TSO from
KindStringandKindBytes. If a user passes an integer variable (e.g.,SELECT ... AS OF TIMESTAMP@int_var`` where@int_varis set as an integer), it would fail since integer Datum kinds (`KindInt64`, `KindUint64`) aren't handled in the TSO fallback path.This may be acceptable if the primary use case is string literals from SQL, but worth considering for completeness.
Optional enhancement to handle integer kinds
// If datetime conversion failed, try to parse as a TiDB TSO (not a Unix timestamp). // A TiDB TSO encodes a physical timestamp (ms since epoch) in the high bits and a logical // counter in the low 18 bits. We validate that the physical component represents a // reasonable time (after 2013-01-01). + var tso uint64 + var parseErr error if tsVal.Kind() == types.KindString || tsVal.Kind() == types.KindBytes { - if tso, err := strconv.ParseUint(tsVal.GetString(), 10, 64); err == nil { - physicalMS := oracle.ExtractPhysical(tso) - // 1356998400000 ms = 2013-01-01T00:00:00Z, a reasonable lower bound for any TiDB TSO. - if physicalMS > 1356998400000 { - return tso, nil - } - } + tso, parseErr = strconv.ParseUint(tsVal.GetString(), 10, 64) + } else if tsVal.Kind() == types.KindInt64 || tsVal.Kind() == types.KindUint64 { + tso, parseErr = tsVal.GetUint64(), nil + } + if parseErr == nil && tso > 0 { + physicalMS := oracle.ExtractPhysical(tso) + // 1356998400000 ms = 2013-01-01T00:00:00Z, a reasonable lower bound for any TiDB TSO. + if physicalMS > 1356998400000 { + return tso, nil + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessiontxn/staleread/util.go` around lines 69 - 77, The tsVal TSO parsing only handles types.KindString and types.KindBytes, so add handling for integer Datum kinds (types.KindInt64 and types.KindUint64) in the same fallback block: if tsVal.Kind() == types.KindInt64 or types.KindUint64, read the numeric TSO via tsVal.GetInt64()/tsVal.GetUint64() (casting to uint64 as needed), call oracle.ExtractPhysical on that value, validate against the same lower bound (1356998400000), and return the tso if valid; keep the existing string/bytes parsing path unchanged and ensure the same error/return behavior if validation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/sessiontxn/staleread/util.go`:
- Around line 69-77: The tsVal TSO parsing only handles types.KindString and
types.KindBytes, so add handling for integer Datum kinds (types.KindInt64 and
types.KindUint64) in the same fallback block: if tsVal.Kind() == types.KindInt64
or types.KindUint64, read the numeric TSO via tsVal.GetInt64()/tsVal.GetUint64()
(casting to uint64 as needed), call oracle.ExtractPhysical on that value,
validate against the same lower bound (1356998400000), and return the tso if
valid; keep the existing string/bytes parsing path unchanged and ensure the same
error/return behavior if validation fails.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/sessiontxn/staleread/processor_test.gopkg/sessiontxn/staleread/util.gotests/integrationtest/r/executor/stale_txn.resulttests/integrationtest/t/executor/stale_txn.test
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
pkg/sessiontxn/staleread/util.go
Outdated
| // A TiDB TSO encodes a physical timestamp (ms since epoch) in the high bits and a logical | ||
| // counter in the low 18 bits. We validate that the physical component represents a | ||
| // reasonable time (after 2013-01-01). | ||
| if tsVal.Kind() == types.KindString || tsVal.Kind() == types.KindBytes { |
There was a problem hiding this comment.
[P2] TSO parsing ignores non-string integer-like values
Why: The TSO fallback only triggers when the evaluated datum kind is KindString or KindBytes. If a user passes an integer-valued expression — e.g. AS OF TIMESTAMP CAST(@tso AS UNSIGNED), arithmetic on a TSO, or JSON_EXTRACT(...) returning a number — the datetime parse fails and the error is returned directly without attempting TSO parsing. This makes the feature incomplete and surprising, since the PR's stated goal is to accept raw TSO integers.
Evidence: pkg/sessiontxn/staleread/util.go:69
if tsVal.Kind() == types.KindString || tsVal.Kind() == types.KindBytes {
if tso, err := strconv.ParseUint(tsVal.GetString(), 10, 64); ...
}Non-string kinds (KindInt64, KindUint64, KindMysqlJSON) fall through to return 0, datetimeErr without TSO fallback. Note: this also affects AS OF TIMESTAMP TIDB_CURRENT_TSO() (int-returning builtin) unless explicitly cast to string.
There was a problem hiding this comment.
The master agent encountered an error while processing. Please try again later.
ℹ️ Learn more details on Pantheon AI.
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
pkg/sessiontxn/staleread/util.go
Outdated
| // A TiDB TSO encodes a physical timestamp (ms since epoch) in the high bits and a logical | ||
| // counter in the low 18 bits. We validate that the physical component represents a | ||
| // reasonable time (after 2013-01-01). | ||
| if tsVal.Kind() == types.KindString || tsVal.Kind() == types.KindBytes { |
pkg/sessiontxn/staleread/util.go
Outdated
| physicalMS := oracle.ExtractPhysical(tso) | ||
| // 1356998400000 ms = 2013-01-01T00:00:00Z, a reasonable lower bound for any TiDB TSO. | ||
| if physicalMS > 1356998400000 { | ||
| return tso, nil |
There was a problem hiding this comment.
Need to validate the upper bounds to avoid future ts, like
ValidateSnapshotReadTS, future ts may break linearizability when async commit is enabled.
There was a problem hiding this comment.
Updated to check this also
|
@MyonKeminta @ekexium |
pkg/sessiontxn/staleread/util.go
Outdated
| } | ||
| } | ||
|
|
||
| return 0, datetimeErr |
There was a problem hiding this comment.
If the user passes in an invalid TSO, the error returned is still datetimeErr. This is acceptable but slightly confusing
There was a problem hiding this comment.
Updated this to return a more specific error
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/sessiontxn/staleread/util.go (1)
72-76: Consider extracting the magic number as a named constant.The value
1356998400000(2013-01-01T00:00:00Z in milliseconds) is used as a lower bound for valid TSOs but isn't self-documenting. A named constant would improve clarity and make it easier to maintain if this threshold ever needs adjustment.Suggested improvement
+// minValidTSOPhysicalMS is the minimum physical timestamp (in milliseconds) for a valid TiDB TSO. +// This corresponds to 2013-01-01T00:00:00Z, a reasonable lower bound predating TiDB's existence. +const minValidTSOPhysicalMS = 1356998400000 + func CalculateAsOfTsExpr(ctx context.Context, sctx planctx.PlanContext, tsExpr ast.ExprNode) (uint64, error) { ... physicalMS := oracle.ExtractPhysical(tso) - // 1356998400000 ms = 2013-01-01T00:00:00Z, a reasonable lower bound for any TiDB TSO - if physicalMS <= 1356998400000 { + if physicalMS <= minValidTSOPhysicalMS { return 0, plannererrors.ErrAsOf.FastGenWithCause("invalid TSO timestamp: TSO is before 2013-01-01") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessiontxn/staleread/util.go` around lines 72 - 76, Extract the hardcoded 1356998400000 literal into a named constant (e.g., minValidTSOPhysicalMS or MinTSOTimeMs) and replace its use in the TSO lower-bound check; update the check that uses physicalMS (and add a short comment explaining that the constant represents 2013-01-01T00:00:00Z in ms) so the intent is clear and the threshold can be adjusted centrally in functions like the TSO validation logic where physicalMS := oracle.ExtractPhysical(tso) is compared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/sessiontxn/staleread/util.go`:
- Around line 72-76: Extract the hardcoded 1356998400000 literal into a named
constant (e.g., minValidTSOPhysicalMS or MinTSOTimeMs) and replace its use in
the TSO lower-bound check; update the check that uses physicalMS (and add a
short comment explaining that the constant represents 2013-01-01T00:00:00Z in
ms) so the intent is clear and the threshold can be adjusted centrally in
functions like the TSO validation logic where physicalMS :=
oracle.ExtractPhysical(tso) is compared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc1f4352-c33f-4e89-866e-9a5890c0d0e6
📒 Files selected for processing (4)
pkg/sessiontxn/staleread/processor_test.gopkg/sessiontxn/staleread/util.gotests/integrationtest/r/executor/stale_txn.resulttests/integrationtest/t/executor/stale_txn.test
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integrationtest/r/executor/stale_txn.result
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/sessiontxn/staleread/util.go (1)
80-82: Avoid validating the same read TS twice on the TSO path.
CalculateAsOfTsExprvalidatestsohere, and the caller inpkg/sessiontxn/staleread/processor.go(Line 280-292) validates again after this function returns. For TSO inputs this adds an extra oracle validation round trip per query. Consider centralizing this check in one layer to avoid duplicate PD/oracle pressure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessiontxn/staleread/util.go` around lines 80 - 82, CalculateAsOfTsExpr currently calls sessionctx.ValidateSnapshotReadTS(ctx, sctx.GetStore(), tso, true) which causes a duplicate oracle/PD validation because its caller in pkg/sessiontxn/staleread/processor.go already validates TSO; remove the ValidateSnapshotReadTS call from CalculateAsOfTsExpr (or change CalculateAsOfTsExpr to accept a skipValidation boolean and bypass the call) and ensure the single responsibility for TSO validation remains in the caller (processor.go) so TSO inputs only trigger one ValidateSnapshotReadTS invocation; update function signature/use sites if you add the skip flag and keep the validation call in processor.go (use sessionctx.ValidateSnapshotReadTS there) as the centralized check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/sessiontxn/staleread/processor_test.go`:
- Around line 238-263: Update TestStaleReadSupportDateTimeAndTSO to include a
regression case that exercises non-string numeric Datum kinds
(KindInt64/KindUint64) by invoking processor.OnSelectTable with an AST that uses
an unquoted numeric expression or CAST(<tso> AS UNSIGNED) for AS OF TIMESTAMP;
use genStaleReadPoint to get a tso value, call createProcessor and OnSelectTable
with astTableWithAsOf using that numeric expression, then assert
processor.GetStalenessReadTS() equals the expected tso to ensure tsoFromDatum
handles non-string numerics.
---
Nitpick comments:
In `@pkg/sessiontxn/staleread/util.go`:
- Around line 80-82: CalculateAsOfTsExpr currently calls
sessionctx.ValidateSnapshotReadTS(ctx, sctx.GetStore(), tso, true) which causes
a duplicate oracle/PD validation because its caller in
pkg/sessiontxn/staleread/processor.go already validates TSO; remove the
ValidateSnapshotReadTS call from CalculateAsOfTsExpr (or change
CalculateAsOfTsExpr to accept a skipValidation boolean and bypass the call) and
ensure the single responsibility for TSO validation remains in the caller
(processor.go) so TSO inputs only trigger one ValidateSnapshotReadTS invocation;
update function signature/use sites if you add the skip flag and keep the
validation call in processor.go (use sessionctx.ValidateSnapshotReadTS there) as
the centralized check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 796e970c-78eb-44a8-a456-a878194da3e0
📒 Files selected for processing (4)
pkg/sessiontxn/staleread/processor_test.gopkg/sessiontxn/staleread/util.gotests/integrationtest/r/executor/stale_txn.resulttests/integrationtest/t/executor/stale_txn.test
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integrationtest/t/executor/stale_txn.test
- tests/integrationtest/r/executor/stale_txn.result
| func TestStaleReadSupportDateTimeAndTSO(t *testing.T) { | ||
| store := testkit.CreateMockStore(t, mockstore.WithStoreType(mockstore.EmbedUnistore)) | ||
| tk := testkit.NewTestKit(t, store) | ||
| p1 := genStaleReadPoint(t, tk) | ||
| p2 := genStaleReadPoint(t, tk) | ||
|
|
||
| require.NotEqual(t, p1.ts, p2.ts) | ||
|
|
||
| // Reading AS OF TIMESTAMP 'TSO' should be parsed correctly. | ||
| processor := createProcessor(t, tk.Session()) | ||
| err := processor.OnSelectTable(astTableWithAsOf(t, fmt.Sprintf("%d", p1.ts))) | ||
| require.NoError(t, err) | ||
| require.Equal(t, processor.GetStalenessReadTS(), p1.ts) | ||
|
|
||
| // Reading AS OF TIMESTAMP 'TSO' does not lose precision. | ||
| processor = createProcessor(t, tk.Session()) | ||
| err = processor.OnSelectTable(astTableWithAsOf(t, fmt.Sprintf("%d", p1.ts+1))) | ||
| require.NoError(t, err) | ||
| require.Equal(t, processor.GetStalenessReadTS(), p1.ts+1) | ||
|
|
||
| // Reading AS OF TIMESTAMP 'YYYY-MM-DD HH:MM:SS' should be parsed correctly. | ||
| processor = createProcessor(t, tk.Session()) | ||
| err = processor.OnSelectTable(astTableWithAsOf(t, p1.dt)) | ||
| require.NoError(t, err) | ||
| require.Equal(t, processor.GetStalenessReadTS(), p1.ts) | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/sessiontxn/staleread/util.go
Outdated
| // 1356998400000 ms = 2013-01-01T00:00:00Z, a reasonable lower bound for any TiDB TSO | ||
| physicalMS := oracle.ExtractPhysical(tso) | ||
| if physicalMS <= 1356998400000 { |
There was a problem hiding this comment.
Better to extract this to a constant
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekexium, nolouch The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
@madhavramesh: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@madhavramesh: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
/ok-to-test |
ti-chi-bot
bot
added
ok-to-test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66643 +/- ##
================================================
- Coverage 77.6779% 76.1785% -1.4995%
================================================
Files 2006 1943 -63
Lines 548699 565385 +16686
================================================
+ Hits 426218 430702 +4484
- Misses 120821 132940 +12119
- Partials 1660 1743 +83
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest-required |
|
@madhavramesh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@madhavramesh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
What problem does this PR solve?
Issue Number: close #64314
Problem Summary: The change allows specifying TSO when using AS OF TIMESTAMP syntax. Currently, AS OF TIMESTAMP only supports datetime, which is asymmetric with tidb_snapshot system variable that accepts both datetime or TSO.
Using tidb_snapshot is inconvenient since it pollute the session if not reverted. Using AS OF TIMESTAMP is much more convenient but lacks precision, which this PRs address.
What changed and how does it work?
If parsing the timestamp as datetime fails, and the expression is a valid integer, parse it as TSO. This approach keeps backwards compatibility as discussed in #64314.
One caveat is that if someone specifies a string in the format 'YYYYMMDDHHMMSS', AS OF TIMESTAMP will treat it as a datetime, while @tidb_snapshot treats as a int. This is an explicit choice to keep backwards compatability.
Check List
Tests
Side effects
Release note
Summary by CodeRabbit