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

core_test: add cases that local temporary table works well for cte #27174

Merged

Conversation

zhaoxugang
Copy link
Contributor

@zhaoxugang zhaoxugang commented Aug 12, 2021

What problem does this PR solve?

Issue Number: close #25951

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

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

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 12, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • djshow832
  • lcwangchao

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 12, 2021
# Conflicts:
#	planner/core/integration_test.go
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2021
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

MySQL:

mysql> drop table t1;
Query OK, 0 rows affected (0.02 sec)

mysql>  create temporary table t1 (i int);
Query OK, 0 rows affected (0.01 sec)

mysql>  insert into t1 values (5),(4),(1),(2),(3);
Query OK, 5 rows affected (0.01 sec)
Records: 5  Duplicates: 0  Warnings: 0
mysql> with cte as (select * from t1) select * from cte;
+------+
| i    |
+------+
|    5 |
|    4 |
|    1 |
|    2 |
|    3 |
+------+
5 rows in set (0.00 sec)

@zhaoxugang
Copy link
Contributor Author

/rebuild

@zhaoxugang
Copy link
Contributor Author

/rebuild

b.cteToTblIDMapper[cteName] = append(b.cteToTblIDMapper[cteName], tblIds...)
}
} else {
if tblIds, ok := b.cteToTblIDMapper[tn.Name.L]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like this line is duplicated with 3846, could we simplify the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return nil, infoschema.ErrTableNotExists.GenWithStack("Table which ID = %d does not exist.", tblID)
}
errMsg := fmt.Sprintf(errno.MySQLErrName[errno.ErrCantReopenTable].Raw, tb.Meta().Name.L)
return nil, ErrInternal.GenWithStack(errMsg)
Copy link
Contributor

Choose a reason for hiding this comment

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

how about return nil, ErrInternal.GenWithStack(errno.MySQLErrName[errno.ErrCantReopenTable].Raw,tb.Meta().Name.L)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

The correct usage should be:

  1. define a global error object in this package, maybe error.go
errCantReopenTable =  dbterror.ClassTypes.NewStdErr(errno.ErrCantReopenTable, mysql.MySQLErrName[errno.ErrCantReopenTable])
  1. and then use errCantReopenTable.GenWithStackByArgs(tblName) here

In this way, you can get the correct error code 1137, instead of 1815

@zhaoxugang

@lcwangchao
Copy link
Collaborator

lcwangchao commented Aug 16, 2021

The title is misleading, it is not "Forbid cte on the local temporary table". It should be "Forbade refer temporary table more than once in one statement"

@zhaoxugang zhaoxugang changed the title core: Forbid cte on the local temporary table core: Forbade refer temporary table more than once in one statement Aug 16, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2021
@ti-chi-bot ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 19, 2021
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 20, 2021
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 20, 2021
@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 20, 2021
@lcwangchao
Copy link
Collaborator

@zhaoxugang You can just put a None in release note if nothing to write.

@djshow832
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: b5ebd02

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 20, 2021
@djshow832
Copy link
Contributor

djshow832 commented Aug 20, 2021

FAIL: <autogenerated>:1: testSerialSuite.TestTemporaryTableNoNetwork
The CI failed, but probably not related to this PR. Can you take a look? @lcwangchao

@lcwangchao
Copy link
Collaborator

FAIL: <autogenerated>:1: testSerialSuite.TestTemporaryTableNoNetwork
The CI failed, but probably not related to this PR. Can you take a look? @lcwangchao

I think it's caused by #27405

The test has a goroutine:

go func() {
    _, err := session.ResultSetToStringSlice(ctx, tk1.Se, rs)
    blocked <- struct{}{}
    c.Assert(err, NotNil)
}()

The old version will always be blocked because blocked by blocked <- struct{}{} channel has a zero capacity. In a right case, none will consume it.

So the pr #27405 made some change:

- blocked := make(chan struct{})
+ blocked := make(chan struct{}, 1)

The chan will not be blocked any more, but unfortunately, this line in the goroutine function will alway fail:

c.Assert(err, NotNil)

So the test failed. (If we invoked cancel for a context, the best behavior is return an error. I don't know why it doesn't)

But the test failed in my laptop for another reason:

goleak: Errors on successful test run: found unexpected goroutines:

I think it is because #27405 added goleak to detected leaks but sometimes the goroutine did not exist so quickly.

But It is still strange that the context cancel function is called very early, but in my own environment, it fails as a leak for most cases. So I found that: after we call cancel, the goroutine is still blocked in this line:

_, err := session.ResultSetToStringSlice(ctx, tk1.Se, rs)

And this line is blocked by:

err = rs.Close()

So I know that rs.Close() is always blocked until the failpoint is disabled. (I still wonder why we make a blocking close function).

To avoid this error, we just need to remove c.Assert(err, NotNil) wait for the goroutine exited before the end of test.

@ti-chi-bot
Copy link
Member

@zhaoxugang: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

@ti-chi-bot ti-chi-bot merged commit 4fdb614 into pingcap:master Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add cases that local temporary table works well for cte
7 participants