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: add privilege check for lock/unlock stats #46742

Merged
merged 6 commits into from
Sep 8, 2023

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Sep 7, 2023

What problem does this PR solve?

Issue Number: ref #46351

Problem Summary:

What is changed and how it works?

Added privilege check for lock/unlock stats. They require the SELECT and INSERT privileges.

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

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2023
@Rustin170506
Copy link
Member Author

Rustin170506 commented Sep 7, 2023

  1. Start the TiDB cluster: tiup playground nightly --tiflash 0 --db.binpath /Users/hi-rustin/GolandProjects/tidb/bin/tidb-server
  2. Create table and analyze it:
create table t(a int, b int);
insert into t values (1,2), (3,4), (5,6), (7,8);
analyze table t;
show warnings;
  1. Create a user: CREATE USER 'dd'@'localhost' IDENTIFIED BY 'password';
  2. Only give it delete privilege: GRANT DELETE ON test.* TO 'dd'@'localhost'; FLUSH PRIVILEGES;
  3. Try to lock or unlock the table t: lock stats t;/ unlock stats t;
mysql> lock stats t;
ERROR 1142 (42000): INSERT command denied to user 'dd'@'localhost' for table 't'
mysql> unlock stats t;
ERROR 1142 (42000): INSERT command denied to user 'dd'@'localhost' for table 't'
  1. Give it the INSERT privilege: GRANT INSERT ON test.* TO 'dd'@'localhost'; FLUSH PRIVILEGES;
  2. Try to lock or unlock the table t: lock stats t;/ unlock stats t;
mysql> lock stats t;
ERROR 1142 (42000): SELECT command denied to user 'dd'@'localhost' for table 't'
mysql> unlock stats t;
ERROR 1142 (42000): SELECT command denied to user 'dd'@'localhost' for table 't'
  1. Give it the SELECT privilege: GRANT SELECT ON test.* TO 'dd'@'localhost'; FLUSH PRIVILEGES;
  2. Lock table: lock stats t;
  3. Analyze it again: analyze table t;
  4. Check the warnings: show warnings;
mysql> show warnings;
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+
| Level   | Code | Message                                                                                                                                 |
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+
| Note    | 1105 | Analyze use auto adjusted sample rate 1.000000 for table test.t, reason to use this rate is "use min(1, 110000/4) as the sample-rate=1" |
| Warning | 1105 | skip analyze locked table: t                                                                                                            |
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+
  1. Try to lock it again: lock stats t;
mysql> use test;
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Database changed
mysql> lock stats t;
Query OK, 0 rows affected, 1 warning (0.01 sec)

mysql> show warnings;
+---------+------+-----------------------------------+
| Level   | Code | Message                           |
+---------+------+-----------------------------------+
| Warning | 1105 | skip locking locked table: test.t |
+---------+------+-----------------------------------+
1 row in set (0.00 sec)
  1. Show locked stats: show stats_locked;.
mysql> show stats_locked;
ERROR 1142 (42000): SHOW command denied to user 'dd'@'localhost' for table 'stats_table_locked'
  1. Give it the SELECT privilege on mysql.stats_table_locked: GRANT SELECT ON mysql.stats_table_locked TO 'dd'@'localhost'; FLUSH PRIVILEGES;
  2. Show locked stats: show stats_locked;:
mysql> show stats_locked;
+---------+------------+----------------+--------+
| Db_name | Table_name | Partition_name | Status |
+---------+------------+----------------+--------+
| test    | t          |                | locked |
+---------+------------+----------------+--------+
1 row in set (0.00 sec)
  1. Unlock the table: unlock stats t;
  2. Check the mysql.stats_table_locked table again:
mysql> select * from mysql.stats_table_locked;
Empty set (0.00 sec)
  1. Show locked stats: show stats_locked;
mysql> show stats_locked;
Empty set (0.00 sec)
  1. Unlock the table again and check warnings: unlock stats t;
mysql> unlock stats t;
Query OK, 0 rows affected, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+---------------------------------------+
| Level   | Code | Message                               |
+---------+------+---------------------------------------+
| Warning | 1105 | skip unlocking unlocked table: test.t |
+---------+------+---------------------------------------+
1 row in set (0.00 sec)

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #46742 (9686f96) into master (670778e) will decrease coverage by 0.6115%.
Report is 12 commits behind head on master.
The diff coverage is 100.0000%.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #46742        +/-   ##
================================================
- Coverage   73.2873%   72.6758%   -0.6115%     
================================================
  Files          1319       1342        +23     
  Lines        395898     402476      +6578     
================================================
+ Hits         290143     292503      +2360     
- Misses        87268      91409      +4141     
- Partials      18487      18564        +77     
Flag Coverage Δ
integration 27.7844% <64.7058%> (?)
unit 73.2964% <100.0000%> (+0.0091%) ⬆️

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

Components Coverage Δ
dumpling 54.0444% <ø> (ø)
parser 84.9924% <ø> (+0.0010%) ⬆️
br 48.2639% <ø> (-4.1995%) ⬇️
📢 Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pingcap).

Copy link
Member Author

@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.

🔢 self-check

planner/core/planbuilder.go Show resolved Hide resolved
@Rustin170506 Rustin170506 added the sig/planner SIG: Planner label Sep 7, 2023
@Rustin170506
Copy link
Member Author

/retest

@qw4990
Copy link
Contributor

qw4990 commented Sep 8, 2023

  1. Start the TiDB cluster: tiup playground nightly --tiflash 0 --db.binpath /Users/hi-rustin/GolandProjects/tidb/bin/tidb-server
  2. Create table and analyze it:
create table t(a int, b int);
insert into t values (1,2), (3,4), (5,6), (7,8);
analyze table t;
show warnings;
  1. Create a user: CREATE USER 'dd'@'localhost' IDENTIFIED BY 'password';
  2. Only give it delete privilege: GRANT DELETE ON test.* TO 'dd'@'localhost'; FLUSH PRIVILEGES;
  3. Try to lock or unlock the table t: lock stats t;/ unlock stats t;
mysql> lock stats t;
ERROR 1142 (42000): INSERT command denied to user 'dd'@'localhost' for table 't'
mysql> unlock stats t;
ERROR 1142 (42000): INSERT command denied to user 'dd'@'localhost' for table 't'
  1. Give it the INSERT privilege: GRANT INSERT ON test.* TO 'dd'@'localhost'; FLUSH PRIVILEGES;
  2. Try to lock or unlock the table t: lock stats t;/ unlock stats t;
mysql> lock stats t;
ERROR 1142 (42000): SELECT command denied to user 'dd'@'localhost' for table 't'
mysql> unlock stats t;
ERROR 1142 (42000): SELECT command denied to user 'dd'@'localhost' for table 't'
  1. Give it the SELECT privilege: GRANT SELECT ON test.* TO 'dd'@'localhost'; FLUSH PRIVILEGES;
  2. Lock table: lock stats t;
  3. Analyze it again: analyze table t;
  4. Check the warnings: show warnings;
mysql> show warnings;
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+
| Level   | Code | Message                                                                                                                                 |
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+
| Note    | 1105 | Analyze use auto adjusted sample rate 1.000000 for table test.t, reason to use this rate is "use min(1, 110000/4) as the sample-rate=1" |
| Warning | 1105 | skip analyze locked table: t                                                                                                            |
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+
  1. Try to lock it again: lock stats t;
mysql> use test;
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Database changed
mysql> lock stats t;
Query OK, 0 rows affected, 1 warning (0.01 sec)

mysql> show warnings;
+---------+------+-----------------------------------+
| Level   | Code | Message                           |
+---------+------+-----------------------------------+
| Warning | 1105 | skip locking locked table: test.t |
+---------+------+-----------------------------------+
1 row in set (0.00 sec)
  1. Show locked stats: show stats_locked;.
mysql> show stats_locked;
ERROR 1142 (42000): SHOW command denied to user 'dd'@'localhost' for table 'stats_table_locked'
  1. Give it the SELECT privilege on mysql.stats_table_locked: GRANT SELECT ON mysql.stats_table_locked TO 'dd'@'localhost'; FLUSH PRIVILEGES;
  2. Show locked stats: show stats_locked;:
mysql> show stats_locked;
+---------+------------+----------------+--------+
| Db_name | Table_name | Partition_name | Status |
+---------+------------+----------------+--------+
| test    | t          |                | locked |
+---------+------------+----------------+--------+
1 row in set (0.00 sec)
  1. Unlock the table: unlock stats t;
  2. Check the mysql.stats_table_locked table again:
mysql> select * from mysql.stats_table_locked;
Empty set (0.00 sec)
  1. Show locked stats: show stats_locked;
mysql> show stats_locked;
Empty set (0.00 sec)
  1. Unlock the table again and check warnings: unlock stats t;
mysql> unlock stats t;
Query OK, 0 rows affected, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+---------------------------------------+
| Level   | Code | Message                               |
+---------+------+---------------------------------------+
| Warning | 1105 | skip unlocking unlocked table: test.t |
+---------+------+---------------------------------------+
1 row in set (0.00 sec)

LGTM, but could you transform this to a unit test like TestTableLockPrivilege?

@Rustin170506
Copy link
Member Author

LGTM, but could you transform this to a unit test like TestTableLockPrivilege?

Good point. I will add it.

@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 Sep 8, 2023
@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 8, 2023
@ti-chi-bot
Copy link

ti-chi-bot bot commented Sep 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei, qw4990

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 8, 2023
@ti-chi-bot
Copy link

ti-chi-bot bot commented Sep 8, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-09-08 06:08:21.233888277 +0000 UTC m=+2684865.782904259: ☑️ agreed by qw4990.
  • 2023-09-08 07:03:33.648127343 +0000 UTC m=+2688178.197143315: ☑️ agreed by hawkingrei.

@ti-chi-bot ti-chi-bot bot merged commit 78d1897 into pingcap:master Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm 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.

3 participants