planner: support table aliases in FOR UPDATE OF Clause#65532
planner: support table aliases in FOR UPDATE OF Clause#65532ti-chi-bot[bot] merged 14 commits intopingcap:masterfrom
Conversation
|
Hi @cryo-zd. 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 @cryo-zd. 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. |
|
/ok-to-test |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #65532 +/- ##
================================================
+ Coverage 77.7132% 79.8590% +2.1458%
================================================
Files 2013 1963 -50
Lines 551161 548252 -2909
================================================
+ Hits 428325 437829 +9504
+ Misses 121105 108949 -12156
+ Partials 1731 1474 -257
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds locking-clause awareness to the preprocessor: collects FROM-clause table refs per SELECT, defers and binds FOR UPDATE / LOCK IN SHARE MODE targets to aliases or qualified names with compatibility warnings, and prefers resolved DBInfo for privilege checks during plan building. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Pre as Preprocessor
participant Catalog as Catalog/DBInfo
participant Planner as LogicalPlanner
Client->>Pre: parse SELECT ... FOR UPDATE
Pre->>Pre: push lockRefCollector & lockTables map
Pre->>Pre: traverse FROM -> collect aliases into collector
Pre->>Pre: mark locking-clause TableName nodes to skip normal resolution
Pre->>Pre: leave SELECT -> checkLockClauseTables() binds targets to collected refs (emit warnings if needed)
Pre->>Catalog: resolve bound targets' DBInfo (if required)
Catalog-->>Pre: return DBInfo (may include Name.L)
Pre->>Planner: pass resolved lock bindings
Planner->>Catalog: use tNameW.DBInfo.Name.L (if present) for privilege checks
Planner-->>Client: return plan with validated lock targets
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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)
📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/planner/core/preprocess.go`:
- Around line 1938-1943: The qualifiedMap population currently overwrites
earlier refs for duplicate qualified keys
(qualifiedMap[src.Schema.L+"."+src.Name.L] = ref) causing inconsistent
resolution versus the unqualified orderedRefs which picks the first match;
change the logic in the block that handles src.Schema.L to only set qualifiedMap
entries if the key does not already exist (i.e., preserve the first-seen ref)
for both the schema.table key and the schema.alias key (x.AsName.L) so that
lookups for qualified names are deterministic and consistent with orderedRefs
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a1dd41f5-3472-4bde-abee-524795b3a78b
📒 Files selected for processing (3)
pkg/planner/core/preprocess.gotests/integrationtest/r/planner/core/issuetest/planner_issue.resulttests/integrationtest/t/planner/core/issuetest/planner_issue.test
|
/retest |
18 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #63035
Problem Summary:
SELECT ... FOR UPDATE OF <table>currently fails to resolve table alias in the locking clause during preprocess, return "table not exists" for valid alias targets.What changed and how does it work?
MySQL: locking tables across databases
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit