pt-query-digest review table privilege checks don't work

[Mrten]

pt-query-digest reports an error about 'INSERT' privileges when the problem is missing DELETE privileges (related to bug 940421?)

# TableParser:8321 5635 Checking `maatkit`.`query_review`
# TableParser:8325 5635 SHOW TABLES FROM `maatkit` LIKE 'query\_review'
# TableParser:8339 5635 Table exists; no privs to check
# TableParser:8343 5635 SHOW FULL COLUMNS FROM `maatkit`.`query_review`
# TableParser:8358 5635 DELETE FROM `maatkit`.`query_review` LIMIT 0
# TableParser:8364 5635 User privs on `maatkit`.`query_review` : select,insert,update
# TableParser:8369 5635 User does not have all privs
The query review table `maatkit`.`query_review` does not exist or you do not have INSERT privileges at /usr/bin/pt-query-digest line 12044.

[Baron Schwartz (baron-xaprb)]

This privilege check doesn't work because there isn't a good way in MySQL to check what privileges you have on a table. The tool can't do what it's trying to do, and the only way to get the tool to run with --review is to comment out the privilege check in the code. Here's an example of what the tool will see when it runs the checks it's trying to do. First, the privileges:

mysql> show grants;
+-----------------------------------------------------------------------------------------------------------+
| Grants for anemometer@% |
+-----------------------------------------------------------------------------------------------------------+
| GRANT SUPER ON *.* TO 'anemometer'@'%' IDENTIFIED BY PASSWORD '*A350DA83CF71149E926E3DAA60377E5E0B3EEBC3' |
| GRANT ALL PRIVILEGES ON `slow_query_log`.* TO 'anemometer'@'%' |
+-----------------------------------------------------------------------------------------------------------+

I have only added SUPER because I have to add --set-vars=binlog_format=row to avoid errors the server throws back otherwise. Anyway, you can see the tool has full privileges on the tables in the slow_query_log database. Now, the tool does this:

 8253 return 1 unless $args{all_privs};
 8254
 8255 $sql = "SHOW FULL COLUMNS FROM $db_tbl";

This returns a table with rows like this:

mysql> show full columns from slow_query_log.global_query_review\G
*************************** 1. row ***************************
     Field: checksum
      Type: bigint(20) unsigned
 Collation: NULL
      Null: NO
       Key: PRI
   Default: NULL
     Extra:
Privileges: select,insert,update,references
   Comment:

So you can see the Privileges column doesn't include DELETE. This we already knew, because the tool handles this case:

 8268 my $privs = $row->{privileges} || $row->{Privileges};
 8269
 8270 $sql = "DELETE FROM $db_tbl LIMIT 0";
 8271 PTDEBUG && _d($sql);
 8272 eval {
 8273 $dbh->do($sql);
 8274 };
 8275 my $can_delete = $EVAL_ERROR ? 0 : 1;

The problem is, that DELETE FROM statement is going to throw an error like this:

mysql> delete from slow_query_log.global_query_review limit 0;
ERROR 1665 (HY000): Cannot execute statement: impossible to write to binary log since BINLOG_FORMAT = STATEMENT and at least one table uses a storage engine limited to row-based logging. InnoDB is limited to row-logging when transaction isolation level is READ COMMITTED or READ UNCOMMITTED.

That's the same error I'm adding the call to set the binlog_format to avoid. The tool isn't distinguishing this error from a lack of privileges to DELETE.

I think it's a losing game to play around with this; handling all the cases correctly (setting the transaction isolation level, maybe setting the SQL_MODE in some cases, setting the binlog format which requires SUPER) is going to just make the code a mess. In some tools we'll need different binlog formats and transaction isolation levels for specific purposes anyway.

I'd suggest to just skip the check, other than checking that the table exists. I know it will suck ...

[m00dawg (tim-moocowproductions)]

Yikes this burned me pretty hardcore this morning. I have to wonder, why even do the check at all and simply handle exceptions as they occur within the script itself?

[Brian Fraser (fraserbn)]

We're removing the priv checks in 2.1.6, since we did the same for pt-table-checksum on 2.1.3 and for pt-table-sync in 2.1.4.

[Shahriyar Rzayev (rzayev-sehriyar)]

Percona now uses JIRA for bug reports so this bug report is migrated to: https://jira.percona.com/browse/PT-515