-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
domain: load all non-public tables into infoschema #55142
Conversation
Hi @joechenrh. 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. 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 kubernetes/test-infra repository. |
Hi @joechenrh. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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 kubernetes-sigs/prow repository. |
/ok-to-test |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #55142 +/- ##
=================================================
- Coverage 72.8514% 56.7243% -16.1271%
=================================================
Files 1598 1748 +150
Lines 444454 627838 +183384
=================================================
+ Hits 323791 356137 +32346
- Misses 100734 247142 +146408
- Partials 19929 24559 +4630
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pkg/infoschema/infoschema_v2.go
Outdated
@@ -1043,6 +1043,14 @@ func loadTableInfo(ctx context.Context, r autoid.Requirement, infoData *Data, tb | |||
)) | |||
} | |||
|
|||
// table is not public | |||
if tblInfo.State != model.StatePublic { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it means something wrong if we see the non-public tables?
Do you have any case to cover it? or this is just 'defensive programming'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the only case is: user would see table being deleted from information_schema.tables
. I'll try to construct this case.
/ok-to-test |
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tiancaiamao, wjhuang2016 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 |
[LGTM Timeline notifier]Timeline:
|
/retest |
2 similar comments
/retest |
/retest |
@joechenrh: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
* 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) ...
What problem does this PR solve?
Issue Number: close #54906
What changed and how does it work?
Currently,
fetchSchemasWithTables
will load non-public schema. This can cause inconsistence between v1 and v2 afterdrop database
and V1V2 consistency check has found several failed cases related to this.Consider the following sequence of operations:
applyDropSchemaV2
will return all tableIDs in this deleted schema whileapplyDropSchema
will return nothing.tidb/pkg/infoschema/infoschema_v2.go
Lines 1246 to 1264 in ebbe53c
Check List
Tests
You can test it using unit test in this PR: #55030
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.