planner: ensure that the path of the plan replayer file generated by the old and new versions of TiDB is the same#66348
planner: ensure that the path of the plan replayer file generated by the old and new versions of TiDB is the same#66348zeminzhou wants to merge 2 commits intopingcap:masterfrom
Conversation
… by the old and new versions of TiDB is the same Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
ti-chi-bot
bot
added
do-not-merge/needs-tests-checked
do-not-merge/needs-triage-completed
release-note-none
|
Posted PR review with inline comments: #66348 (review) Note: |
ti-chi-bot
bot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
sig/planner
|
Hi @zeminzhou. 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66348 +/- ##
================================================
- Coverage 77.6822% 77.4829% -0.1994%
================================================
Files 2006 1928 -78
Lines 548533 536949 -11584
================================================
- Hits 426113 416044 -10069
- Misses 120759 120899 +140
+ Partials 1661 6 -1655
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
| timeStr := now.Format("20060102150405") | ||
| filename := fmt.Sprintf("test_%s.txt", timeStr) | ||
| path = filepath.Join(path, filename) | ||
| dir = filepath.Join(dir, "replayer") |
There was a problem hiding this comment.
P2: Storage root can change across upgrades causing 404s
The local root selection depends on whether <logDir>/replayer/ exists. If this directory is not pre-created (even when <logDir> is writable), extstore falls back to TempDir. This means:
- After upgrade, the storage root can change from
<logDir>toTempDir(or vice versa) - Previously generated diagnostic artifacts (extract, optimizer trace, plan replayer files) will appear missing
- Download endpoints will return 404 for existing files stored under the old root
Recommendation: Consider auto-creating <logDir>/replayer during initialization, or implement fallback logic to check multiple root locations during file lookups.
| tidbLogDir := filepath.Dir(config.GetGlobalConfig().Log.File.Filename) | ||
| tidbLogDir = filepath.Clean(tidbLogDir) | ||
| if canWriteToFile(fs, tidbLogDir) { | ||
| if canWriteToReplayerDirFile(fs, tidbLogDir) { |
There was a problem hiding this comment.
P2: Non-writable log base can cause diagnostic feature failures
The writability check tests <logDir>/replayer/ but returns <logDir> as the storage root. Other extstore consumers (extract, optimizer_trace) need to create their own top-level subdirectories under <logDir>.
In permission layouts where:
<logDir>is not writable<logDir>/replayerIS writable (pre-created/mounted/symlinked)
These operations will fail with permission errors when trying to create <logDir>/extract/ or <logDir>/optimizer_trace/.
Recommendation: Either check writability of <logDir> itself, or have canWriteToReplayerDirFile return <logDir>/replayer instead of <logDir>.
| tidbLogDir := filepath.Dir(config.GetGlobalConfig().Log.File.Filename) | ||
| tidbLogDir = filepath.Clean(tidbLogDir) | ||
| if canWriteToFile(fs, tidbLogDir) { | ||
| if canWriteToReplayerDirFile(fs, tidbLogDir) { |
There was a problem hiding this comment.
P2: Keyspace/namespace directory path not validated
NewExtStorage appends keyspaceName to the local base path, creating <logDir>/<keyspace> as the actual storage root. However, canWriteToReplayerDirFile only validates <logDir>/replayer/, not <logDir>/<keyspace>/.
If <logDir>/replayer is writable but <logDir> does not have permissions to create new sibling directories, the extstore initialization will fail when trying to mkdirAll(<logDir>/<keyspace>).
Impact: Breaks plan-replayer, trace, and extract features in keyspace-enabled deployments with restrictive directory permissions.
Recommendation: The writability probe should test the actual target path including the keyspace namespace.
|
/check-issue-triage-complete |
|
/run-check-issue-triage-complete |
1 participant
What problem does this PR solve?
Issue Number: close #66347
Problem Summary:
What changed and how does it work?
Root cause
getLocalPathDirName() in pkg/planner/extstore/extstore.go decides the local storage root for plan replayer when no cloud storage URI is set. Plan replayer then writes objects under GetPlanReplayerDirName() (i.e. the "replayer" segment) relative to that root. So the effective path is <getLocalPathDirName()>/replayer/. For backward compatibility, this must match the old layout: <log_dir>/replayer/, i.e. the storage root must be the log directory, not a path that already includes replayer.
Fix
Ensure getLocalPathDirName() returns the log directory (filepath.Dir(config.GetGlobalConfig().Log.File.Filename)), same as the old behavior, so the storage root is unchanged.
Keep writability checks so we only use this log dir when we can actually write there (or when we can write under the replayer subdir, depending on the chosen check).
No change to how plan replayer builds object keys: it still uses replayer/, so the final path remains <log_dir>/replayer/ and matches old TiDB.
Result
Old and new TiDB versions use the same plan replayer file location for local storage, so upgrades and path-dependent tooling remain compatible.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.