Skip to content

planner: keep partition processor for static pruning#68541

Open
hawkingrei wants to merge 5 commits into
pingcap:masterfrom
hawkingrei:issue-57861
Open

planner: keep partition processor for static pruning#68541
hawkingrei wants to merge 5 commits into
pingcap:masterfrom
hawkingrei:issue-57861

Conversation

@hawkingrei

@hawkingrei hawkingrei commented May 21, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: close #57861

Problem Summary:

In static partition prune mode, partition_processor is required to rewrite a logical partitioned table scan into concrete partition scans. If the rule is disabled through mysql.opt_rule_blacklist, SELECT ... FOR UPDATE can be planned over the logical table scan and return an empty result even though the row exists in a partition.

What changed and how does it work?

This PR treats PartitionProcessor as a correctness rule when the statement uses static partition pruning. In that mode, the logical optimizer ignores the opt-rule blacklist entry for partition_processor, emits a warning, and still runs the static partition rewrite. Dynamic partition pruning keeps the existing blacklist behavior.

It also adds a regression test for a hash-partitioned table with a virtual generated column, SELECT ... FOR UPDATE, and both predicate_push_down and partition_processor listed in mysql.opt_rule_blacklist.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Test command:

go test --tags=intest ./pkg/executor -run TestSelectLockWithStaticPruneAndPartitionProcessorBlacklist -count=1

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Fix an issue that `SELECT ... FOR UPDATE` on partitioned tables could return empty results in static partition pruning mode when the partition processor rule is disabled.

Summary by CodeRabbit

  • Tests

    • Added a test validating SELECT ... FOR UPDATE with static partition pruning when optimizer rules are blacklisted, ensuring queries on partitioned and non-partitioned tables return expected rows.
  • Bug Fixes

    • Ensured the partition-processor rule is effectively enabled for correctness under static pruning and emits a deduplicated warning when a blacklist override occurs.

Review Change Stack

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-tests-checked release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 21, 2026
@ti-chi-bot

ti-chi-bot Bot commented May 21, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign time-and-fate for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-triage-completed sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 21, 2026
@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e136a1cf-fc86-4dc1-ba34-515e98470427

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf7e30 and f4f96d1.

📒 Files selected for processing (2)
  • pkg/executor/partition_table_test.go
  • pkg/planner/core/optimizer.go

📝 Walkthrough

Walkthrough

Make logical-rule disablement plan-aware: optimizer now passes the current logical plan to rule-disable checks and forces PartitionProcessor active when static partition pruning requires it (emitting a one-time warning). Adds a test that exercises static prune with mysql.opt_rule_blacklist.

Changes

Plan-aware logical rule disabling

Layer / File(s) Summary
Optimizer: plan-aware rule-disable checks
pkg/planner/core/optimizer.go
isLogicalRuleDisabled now accepts the current logical plan; call sites in normalizeOptimize, logicalOptimize, and interaction re-opt scheduling pass the logical plan. Special-case *rule.PartitionProcessor: when static prune mode is active, it is not disabled and a one-time warning is appended.
Executor test for static prune + blacklist
pkg/executor/partition_table_test.go
New test TestSelectLockWithStaticPruneAndPartitionProcessorBlacklist sets @@session.tidb_partition_prune_mode='static', manipulates mysql.opt_rule_blacklist (clear, reload, then insert predicate_push_down and partition_processor), runs SELECT ... FOR UPDATE before/after, and asserts the expected warning about partition_processor being ignored under static prune.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Possibly related PRs

  • pingcap/tidb#68425: Both PRs touch partition-processor/partition-pruning behavior; this PR makes rule disabling plan-aware and warns when partition_processor is ignored in static prune mode.

Suggested labels

size/S, approved, lgtm

Suggested reviewers

  • AilinKid
  • qw4990

Poem

I’m a rabbit through optimizer trees, I hop on rules with care,
When static pruning needs PartitionProcessor, I keep it there,
A blacklist tries to hide it, but I leave a tiny note,
A warning like a carrot tied to correctness on my coat,
Hoppity-hop — the plans stay sound, and queries still can float. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately summarizes the main change: keeping the partition processor rule enabled for static partition pruning despite blacklist entries.
Description check ✅ Passed PR description covers problem statement, solution approach, test details, and release notes following the template structure, though some optional sections are incomplete.
Linked Issues check ✅ Passed Changes directly address the root cause in issue #57861: treating partition_processor as a correctness rule in static pruning mode to prevent empty result sets.
Out of Scope Changes check ✅ Passed Both changes focus on the partition processor and static pruning logic described in the linked issue; no unrelated or out-of-scope modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/planner/core/optimizer.go`:
- Around line 1021-1022: The code currently appends interactionRule to
againRuleList when isLogicalRuleDisabled(interactionRule, logic) is true, which
is inverted — change the condition so the rule is re-queued only when it is NOT
disabled; specifically, in the block that tests planChanged && ok &&
isLogicalRuleDisabled(interactionRule, logic), replace the disabled check with
its negation (or call an isLogicalRuleEnabled equivalent) so that
interactionRule is appended only when the rule is enabled.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 857c7314-6e52-4d86-9735-67ba55ece808

📥 Commits

Reviewing files that changed from the base of the PR and between ae3b70a and 58d55de.

📒 Files selected for processing (2)
  • pkg/executor/partition_table_test.go
  • pkg/planner/core/optimizer.go

Comment thread pkg/planner/core/optimizer.go Outdated
@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.80645% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.4408%. Comparing base (ae3b70a) to head (f4f96d1).

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #68541        +/-   ##
================================================
- Coverage   76.3873%   75.4408%   -0.9466%     
================================================
  Files          2038       2020        -18     
  Lines        562779     565943      +3164     
================================================
- Hits         429892     426952      -2940     
- Misses       131969     138987      +7018     
+ Partials        918          4       -914     
Flag Coverage Δ
integration 41.4499% <25.8064%> (+1.6710%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 60.4679% <ø> (ø)
parser ∅ <ø> (∅)
br 49.9201% <ø> (-13.0269%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/planner/core/optimizer.go`:
- Around line 1041-1050: The override/warning in isLogicalRuleDisabled currently
only checks session dynamic-prune flag so it triggers for non-partitioned plans;
change isLogicalRuleDisabled (and the branch handling rule.PartitionProcessor)
to first scan the provided logical plan `logic` for any partitioned-scan nodes
(e.g., any node representing a partitioned table/partitioned table scan) and
only if such a node is present and UseDynamicPruneMode is false call
appendPartitionProcessorStaticPruneBlacklistWarning and return false; otherwise
treat the rule as disabled (return true). Ensure the detection traverses the
logical plan tree starting at `logic` (or use an existing helper that detects
partitioned scans) so the override and warning are plan-aware.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9ec17418-8ace-4e0f-8dc7-a1817808b9df

📥 Commits

Reviewing files that changed from the base of the PR and between 58d55de and 0cf7e30.

📒 Files selected for processing (2)
  • pkg/executor/partition_table_test.go
  • pkg/planner/core/optimizer.go

Comment thread pkg/planner/core/optimizer.go
@guo-shaoge guo-shaoge self-requested a review May 21, 2026 07:20
@hawkingrei hawkingrei requested a review from qw4990 May 21, 2026 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Correction Bugfix by AI release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent result after disabling some optimization rules

1 participant