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

Checks for the region label id should be more robust #33665

Closed
CharlesCheung96 opened this issue Apr 2, 2022 · 8 comments · Fixed by #34129
Closed

Checks for the region label id should be more robust #33665

CharlesCheung96 opened this issue Apr 2, 2022 · 8 comments · Fixed by #34129
Assignees
Labels
affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.0 component/pd severity/minor type/bug The issue is confirmed as a bug.

Comments

@CharlesCheung96
Copy link
Contributor

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

➜  curl -X POST http://127.0.0.1:2379/pd/api/v1/config/region-label/rule -H "Content-Type: application/json" --data-binary @- <<DATA
{ "id": "tidb/meta/basic",
   "labels": [{"key": "data-type", "value": "meta"}],
   "rule_type": "key-range",
    "data": [{
         "start_key": "6d00000000000000f8",
         "end_key": "6e00000000000000f8"
   }]
}
DATA

➜  curl -X POST http://127.0.0.1:2379/pd/api/v1/config/region-label/rule -H "Content-Type: application/json" --data-binary @- <<DATA
{ "id": "tidb/meta",
   "labels": [{"key": "data-type", "value": "meta"}],
   "rule_type": "key-range",
    "data": [{
         "start_key": "6d00000000000000f8",
         "end_key": "6e00000000000000f8"
   }]
}
DATA

Run the following command from the mysql client:

select * from information_schema.attributes;
ERROR 1105 (HY000): invalid label rule ID: tidb/meta
ERROR:

2. What did you expect to see? (Required)

+-----------------+-----------+------------------+------------------------------------------+
| ID              | TYPE      | ATTRIBUTES       | RANGES                                   |
+-----------------+-----------+------------------+------------------------------------------+
| tidb/meta/basic | key-range | "data-type=meta" | [6d00000000000000f8, 6e00000000000000f8] |
| tidb/meta       | key-range | "data-type=meta" | [6d00000000000000f8, 6e00000000000000f8] |
+-----------------+-----------+------------------+------------------------------------------+

or something like

+-----------------+-----------+------------------+------------------------------------------+
| ID              | TYPE      | ATTRIBUTES       | RANGES                                   |
+-----------------+-----------+------------------+------------------------------------------+
| tidb/meta/basic | key-range | "data-type=meta" | [6d00000000000000f8, 6e00000000000000f8] |
+-----------------+-----------+------------------+------------------------------------------+
WARNING: invalid label rule ID: tidb/meta

3. What did you see instead (Required)

ERROR 1105 (HY000): invalid label rule ID: tidb/meta
ERROR:

4. What is your TiDB version? (Required)

Release Version: v5.4.0
Edition: Community
Git Commit Hash: 55f3b24c1c9f506bd652ef1d162283541e428872
Git Branch: heads/refs/tags/v5.4.0
UTC Build Time: 2022-01-25 08:41:03
GoVersion: go1.16.4
Race Enabled: false
TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
Check Table Before Drop: false |
@CharlesCheung96 CharlesCheung96 added the type/bug The issue is confirmed as a bug. label Apr 2, 2022
@nolouch
Copy link
Member

nolouch commented Apr 2, 2022

cc @rleungx

@rleungx
Copy link
Member

rleungx commented Apr 2, 2022

It is expected behavior, we recommend using TiDB to change attributes and if you want to use PD API in a TiDB cluster, the ID should meet the TiDB encoding rule.

@nolouch
Copy link
Member

nolouch commented Apr 2, 2022

TiDB needs schema/{DB}/{Table}/{PartitionID} to be rule ID.

func checkRules(ctx context.Context, sctx sessionctx.Context, filter inspectionFilter, rules []ruleChecker) []inspectionResult {
var results []inspectionResult
exec := sctx.(sqlexec.RestrictedSQLExecutor)
for _, rule := range rules {
if !filter.enable(rule.getItem()) {
continue
}
sql := rule.genSQL(filter.timeRange)
rows, _, err := exec.ExecRestrictedSQL(ctx, nil, sql)
if err != nil {
sctx.GetSessionVars().StmtCtx.AppendWarning(fmt.Errorf("execute '%s' failed: %v", sql, err))
continue
}
for _, row := range rows {
results = append(results, rule.genResult(sql, row))
}
}
return results
}

@CharlesCheung96
Copy link
Contributor Author

It is expected behavior, we recommend using TiDB to change attributes and if you want to use PD API in a TiDB cluster, the ID should meet the TiDB encoding rule.

Thanks for the explanation, I understand this behavior, but the question is whether it makes more sense for tidb to display the legal row before reporting an error when there is an illegal id. For the above example:

+-----------------+-----------+------------------+------------------------------------------+
| ID              | TYPE      | ATTRIBUTES       | RANGES                                   |
+-----------------+-----------+------------------+------------------------------------------+
| tidb/meta/basic | key-range | "data-type=meta" | [6d00000000000000f8, 6e00000000000000f8] |
+-----------------+-----------+------------------+------------------------------------------+
ERROR 1105 (HY000): invalid label rule ID: tidb/meta

@rleungx
Copy link
Member

rleungx commented Apr 2, 2022

Because the attributes are only supported in the table level now, the above example breaks this assumption.

@nolouch
Copy link
Member

nolouch commented Apr 2, 2022

I think

It is expected behavior, we recommend using TiDB to change attributes and if you want to use PD API in a TiDB cluster, the ID should meet the TiDB encoding rule.

Thanks for the explanation, I understand this behavior, but the question is whether it makes more sense for tidb to display the legal row before reporting an error when there is an illegal id. For the above example:

+-----------------+-----------+------------------+------------------------------------------+
| ID              | TYPE      | ATTRIBUTES       | RANGES                                   |
+-----------------+-----------+------------------+------------------------------------------+
| tidb/meta/basic | key-range | "data-type=meta" | [6d00000000000000f8, 6e00000000000000f8] |
+-----------------+-----------+------------------+------------------------------------------+
ERROR 1105 (HY000): invalid label rule ID: tidb/meta

I think warning is ok.

@CharlesCheung96
Copy link
Contributor Author

CharlesCheung96 commented Apr 2, 2022

I think

It is expected behavior, we recommend using TiDB to change attributes and if you want to use PD API in a TiDB cluster, the ID should meet the TiDB encoding rule.

Thanks for the explanation, I understand this behavior, but the question is whether it makes more sense for tidb to display the legal row before reporting an error when there is an illegal id. For the above example:

+-----------------+-----------+------------------+------------------------------------------+
| ID              | TYPE      | ATTRIBUTES       | RANGES                                   |
+-----------------+-----------+------------------+------------------------------------------+
| tidb/meta/basic | key-range | "data-type=meta" | [6d00000000000000f8, 6e00000000000000f8] |
+-----------------+-----------+------------------+------------------------------------------+
ERROR 1105 (HY000): invalid label rule ID: tidb/meta

I think warning is ok.

In the latest version, tidb reports decode errors due to changes in this pr.

MySQL [INFORMATION_SCHEMA]> select * from information_schema.attributes;
ERROR 1105 (HY000): decode tableID from key m in rule tidb/meta/basic failed

@AndreMouche
Copy link
Contributor

I'd like to fix this issue. we will do the following thing on TiDB:

  1. TiDB only shows the table labels which are created by tidb(which means with legal TIDB ID)

For table labels which are not created by tidb, if the client want to show them in tidb, please use the same encode algorithm like tidb

@AndreMouche AndreMouche self-assigned this Apr 20, 2022
@overvenus overvenus added affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.0 labels Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.0 component/pd severity/minor type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants