List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:November 19 2010 12:25pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (Georgi.Kodinov:3128)
Bug#57551
View as plain text  
Hi Goergi,

On 11/19/10 9:24 AM, Georgi Kodinov wrote:

[..]

>>
>> FLUSH PRIVILEGES does not seem to be necessary here.
>
> No, it's not. But I want to test it.

Then add a comment.

>>> +--exec $MYSQL_UPGRADE --skip-verbose --force 2>&1
>>> +--query_vertical SELECT
>>> Host,User,Proxied_host,Proxied_user,With_grant FROM
>>> mysql.proxies_priv
>>
>> Tests that use mysql_upgrade should probably source
>> include/mysql_upgrade_preparation.inc
>
> Right. Fixed. thanks for spotting this.
>
>>> +FLUSH PRIVILEGES; + +--echo End of 5.5 tests
>>>
>>> === modified file 'scripts/mysql_system_tables_fix.sql' ---
>>> a/scripts/mysql_system_tables_fix.sql	2010-11-02 15:45:26 +0000
>>> +++ b/scripts/mysql_system_tables_fix.sql	2010-11-18 11:37:49
>>> +0000 @@ -643,7 +643,14 @@ drop procedure mysql.die; ALTER TABLE
>>> user ADD plugin char(60) DEFAULT '' NOT NULL,  ADD
>>> authentication_string TEXT NOT NULL; ALTER TABLE user MODIFY
>>> plugin char(60) DEFAULT '' NOT NULL;
>>>
>>> -CREATE TABLE IF NOT EXISTS proxies_priv (Host char(60) binary
>>> DEFAULT '' NOT NULL, User char(16) binary DEFAULT '' NOT NULL,
>>> Proxied_host char(60) binary DEFAULT '' NOT NULL, Proxied_user
>>> char(16) binary DEFAULT '' NOT NULL, With_grant BOOL DEFAULT 0
>>> NOT NULL, Grantor char(77) DEFAULT '' NOT NULL, Timestamp
>>> timestamp, PRIMARY KEY Host
>>> (Host,User,Proxied_host,Proxied_user), KEY Grantor (Grantor) )
>>> engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='User
>>> proxy privileges';
>>
>> Why remove it?
>
> Because it's not necessary. The way it works is that for
> mysql_upgrade the makefiles concatenate mysql_system_tables.sql and
> mysql_system_tables_fix.sql (in this order) and execute them. Since
> mysql_system_tables.sql already has the CREATE TABLE it's not needed
> here.

Add this explanation as a per-file comment.

>>> +-- Need to pre-fill mysql.proxies_priv with access for root even
>>> when upgrading from +-- older versions + +CREATE TEMPORARY TABLE
>>> tmp_proxies_priv LIKE proxies_priv; +INSERT INTO tmp_proxies_priv
>>> VALUES ('localhost', 'root', '', '', TRUE, '', now()); +INSERT
>>> INTO proxies_priv SELECT * FROM tmp_proxies_priv WHERE
>>> @had_proxies_priv_table=0; +DROP TABLE tmp_proxies_priv;
>>
>> Why the temporary table?
>
> Because of the @had_proxies_priv_table condition : this is basically
> an all-or-nothing INSERT IGNORE :-)

Aren't errors ignored anyway? IIRC, this part of the upgrade process is 
"forced".

[..]

>>>
>>> +  DBUG_ASSERT(table); + if (user_to) { /* rename */ @@ -5864,6
>>> +5878,8 @@ static int handle_grant_table(TABLE_LIST
>>> DBUG_ENTER("handle_grant_table"); THD *thd= current_thd;
>>>
>>> +  DBUG_ASSERT(table);
>>
>> Don't think it's necessary, a NULL deference is a pretty strong
>> assert already.
>
> Well, it's better to get an ASSERT imho : it's more telling than just
> a crash on a particular line.

Telling what? There is not even a single comment line nor a explanation 
in the per-file comment. It's just a loose assert with no explanation. 
It will give the same information as if the NULL is deferenced: table 
can not be NULL.

>>> table->use_all_columns(); if (! table_no) // mysql.user table {
>>> @@ -6295,17 +6311,26 @@ static int handle_grant_data(TABLE_LIST
>>> }
>>>
>>> /* Handle proxies_priv table. */ -  if ((found=
>>> handle_grant_table(tables, 5, drop, user_from, user_to))<   0) +
>>> if (!tables[5].table) { -    /* Handle of table failed, don't
>>> touch the in-memory array. */ -    result= -1; +
>>> sql_print_error("Missing system table mysql.proxies_priv. " +
>>> "If this is because you're running on an old database " +
>>> "please run mysql_upgrade");
>>
>> Wouldn't ER_FEATURE_DISABLED be more appropriate here?
>
> Well, it's printing to the log, not returning an error to the user.

The statement is going to succeed then?

Also, the "If this is because you're" looks strange. Asking Paul, he 
suggested we just say:

"Missing system table mysql.proxies_priv; please run mysql_upgrade to 
create it"

Regards,

Davi
Thread
bzr commit into mysql-5.5-bugteam branch (Georgi.Kodinov:3128) Bug#57551Georgi Kodinov19 Nov
  • Re: bzr commit into mysql-5.5-bugteam branch (Georgi.Kodinov:3128)Bug#57551Davi Arnaut19 Nov
Re: bzr commit into mysql-5.5-bugteam branch (Georgi.Kodinov:3128)Bug#57551Davi Arnaut19 Nov