Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner: fix incorrect maintenance of handleColHelper for recursive CTE #55732

Merged
merged 13 commits into from
Sep 6, 2024

Conversation

time-and-fate
Copy link
Member

@time-and-fate time-and-fate commented Aug 28, 2024

What problem does this PR solve?

Issue Number: close #55666

Problem Summary:

If the recursive part of a CTE references tables other than the CTE itself, the b.handleHelper won't be correctly cleared during buildRecursiveCTE().

It's because this function relies on an error from buildSelect() or buildSetOpr() to recognize the boundary between the seed part and the recursive part. But b.handleHelper won't be cleared when an error happens.

When this happens, there will be some wrong and redundant entries in b.handleHelper, which makes users of this field like buildDelete() get the wrong result or panic.

What changed and how does it work?

Since this PR needs to be cherry-picked, I can't change too many.

  • In buildRecursiveCTE(), clear the b.handleHelper if an error is returned from buildSelect() or buildSetOpr().
  • Try to make the b.handleHelper more clear for recursive CTE:
    • Now tryBuildCTE() will always push an entry into handleHelper when successful.
    • Now buildRecursiveCTE() will always push an entry into handleHelper when successful.

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.

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

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/planner SIG: Planner labels Aug 28, 2024
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.1953%. Comparing base (e9124dd) to head (c9d0d47).
Report is 4 commits behind head on master.

Additional details and impacted files
@@                Coverage Diff                @@
##             master     #55732         +/-   ##
=================================================
- Coverage   72.8514%   56.1953%   -16.6561%     
=================================================
  Files          1601       1729        +128     
  Lines        445302     622110     +176808     
=================================================
+ Hits         324409     349597      +25188     
- Misses       100894     248845     +147951     
- Partials      19999      23668       +3669     
Flag Coverage Δ
integration 37.4003% <100.0000%> (?)
unit 72.1316% <100.0000%> (+0.1704%) ⬆️

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

Components Coverage Δ
dumpling 52.9567% <ø> (ø)
parser ∅ <ø> (∅)
br 52.8045% <ø> (+7.0278%) ⬆️

@ti-chi-bot ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 28, 2024
@XuHuaiyu
Copy link
Contributor

Please add a description of how to handle CTE in the comments for the handleHelper attribute.

@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/needs-triage-completed labels Aug 30, 2024
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 30, 2024
@time-and-fate
Copy link
Member Author

/retest

@Rustin170506 Rustin170506 self-requested a review September 3, 2024 03:54
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks! 👍

@@ -7000,14 +7030,11 @@ func (b *PlanBuilder) buildRecursiveCTE(ctx context.Context, cte ast.ResultSetNo
// Build seed part plan.
saveSelect := x.SelectList.Selects
x.SelectList.Selects = x.SelectList.Selects[:i]
// We're rebuilding the seed part, so we pop the result we built previously.
Copy link
Member

Choose a reason for hiding this comment

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

Can I ask why we need to remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The popMap() here is moved right after b.buildSelect() and b.buildSetOpr() in this function.
This should make it more clear and explicit that we directly drop the entries pushed by them.

@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 4, 2024
@ti-chi-bot ti-chi-bot bot added needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. labels Sep 4, 2024
Copy link

ti-chi-bot bot commented Sep 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rustin170506, winoros

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

The pull request process is described here

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 lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 5, 2024
Copy link

ti-chi-bot bot commented Sep 5, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-09-04 06:59:30.400385092 +0000 UTC m=+427694.918438014: ☑️ agreed by Rustin170506.
  • 2024-09-05 17:34:32.032588724 +0000 UTC m=+552196.550641648: ☑️ agreed by winoros.

@time-and-fate
Copy link
Member Author

/retest

1 similar comment
@time-and-fate
Copy link
Member Author

/retest

@ti-chi-bot ti-chi-bot bot merged commit c011164 into pingcap:master Sep 6, 2024
23 checks passed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.5: #55896.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Sep 6, 2024
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.1: #55897.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Sep 6, 2024
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Sep 6, 2024
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.5: #55898.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Sep 6, 2024
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-8.1: #55899.

ti-chi-bot bot pushed a commit that referenced this pull request Sep 6, 2024
ti-chi-bot bot pushed a commit that referenced this pull request Sep 6, 2024
ti-chi-bot bot pushed a commit that referenced this pull request Sep 6, 2024
ti-chi-bot bot pushed a commit that referenced this pull request Sep 6, 2024
breezewish added a commit to breezewish/tidb that referenced this pull request Sep 8, 2024
* origin/master: (33 commits)
  build(deps): bump github.com/prometheus/common from 0.55.0 to 0.57.0 (pingcap#55762)
  build(deps): bump golang.org/x/sys from 0.24.0 to 0.25.0 (pingcap#55894)
  planner: fix incorrect estRows with global index and json column (pingcap#55842)
  ddl, stat: switch to new struct for add/truncate/drop partition (pingcap#55893)
  planner: hide instance plan cache eviction log if no plan is evicted (pingcap#55918)
  expression: support tidb encode key function (pingcap#51678)
  planner: fix incorrect maintenance of `handleColHelper` for recursive CTE (pingcap#55732)
  executor: some code refine of hash join v2 (pingcap#55887)
  infoschema, meta: fix wrong auto id after `rename table` (pingcap#55847)
  ddl/ingest: set `minCommitTS` when detect remote duplicate keys (pingcap#55588)
  planner: move index advisor into the kernel (pingcap#55874)
  ddl, stats: switch to new struct for create/truncate table (pingcap#55811)
  executor: avoid new small objects in probe stage of hash join v2 (pingcap#55855)
  *: Add tidbCPU/tikvCPU into system tables (pingcap#55455)
  ddl: use static contexts in `NewReorgCopContext` (pingcap#55823)
  executor: fix index out of range bug in hash join v2 (pingcap#55864)
  executor: record index usage for the clustered primary keys (pingcap#55602)
  domain: load all non-public tables into infoschema (pingcap#55142)
  test: fix unstable TestShowViewPriv (pingcap#55868)
  ttl: add `varbinary` case for `TestSplitTTLScanRangesWithBytes` (pingcap#55863)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nil pointer error
5 participants