Re: bzr commit into mysql-6.0-backup branch (cbell:2676) WL#4073

[Øystein Grøvlen]

Chuck Bell wrote:
> Oystein,
> 
> Answering interdelinia... 

Thanks for answers, the only issues I have, in addition to you checking 
up the encoding issue, is test comments that still I do not feel are 
correct (see below).  However, I did not get an answer to why you have 
added drop user statements to some existing tests.

--
Øystein


> 
>> I am not not very familiar with the semantics of grant and revoke, so
>> I guess my review is more questions than comments.   So far I have not
>> found anything severe, but I would like to get answers to my questions
>> before approving the patch.
> ...
> 
>>> === modified file 'mysql-test/r/backup_views.result'
>>> --- a/mysql-test/r/backup_views.result	2008-08-05 
>> 08:04:30 +0000
>>> +++ b/mysql-test/r/backup_views.result	2008-08-07 
>> 14:53:39 +0000
>>> @@ -239,10 +239,10 @@ DROP DATABASE bup_db1;
>>>  DROP DATABASE bup_db2;
>>>  restore database with view dependency to other, non-existing db
>>>  RESTORE FROM 'bup_objectview1.bak';
>>> -ERROR 42S02: Table 'bup_db2.t2' doesn't exist
>>> +ERROR HY000: Could not restore view `bup_db1`.`v5`
>> The ideal here would be to get an error message that contained the
>> information from both the old and the new message.  Would that be
>> possible?
> 
> I don't think so -- not unless we change the code a bit. This part of the
> patch was included because my change caused this issue to crop up. This
> isn't strictly part of my patch but since my patch 'broke' it... I think the
> new message is more correct because there are a number of things that can
> cause a view to fail creation. I would agree that the information from both
> is useful, but I don't think I can capture it. I could add this:
> 
> Could not restore view ... Please check the view definition for possible
> missing dependencies.
> 
> How is that?

Looks better to me.

> 
> ...
> 
>>> +--disable_warnings
>>> +DROP DATABASE IF EXISTS bup_db_grants;
>>> +DROP DATABASE IF EXISTS db2;
>>> +--enable_warnings
>>> +
>>> +--echo Create two databases and two tables.
>> Two?  I see only one.
> 
> bup_db_grants and db2 -- t1 and s1.
> 

As far as I can tell, db2 is not created in this test.

>>> +--echo Now create users and assign various rights to the databases
>>> +CREATE USER 'bup_user1'@'%';
>>> +CREATE USER 'bup_user2'@'%';
>>> +CREATE USER 'bup_user3'@'%';
>> Since part of the new code is handling ticks, it would be great if you
>> could do some testing of user both with and without ticks.
> 
> Yeah, I had that and thought better of it... I can add that back.

Good.

> 
> ...
> 
>>> +--echo Demonstrate rights of the users.
>>> +# Note: Since db2 was not in the backup and the user was 
>> deleted prior to 
>>> +# the backup, that privilege will not appear here.
>> I guess you mean 'prior to the restore'.
> 
> No, I mean backup. Since the information was captured in the backup image.
> 

I do not see any 'drop user' prior to backup.

> ...
> 
>>> +ER_BACKUP_GRANT_SKIPPED
>>> +        eng "The grant '%-.64s' was skipped because the 
>> user does not exist."
>>> +ER_BACKUP_GRANT_WRONG_DB
>>> +        eng "The grant '%-.64s' failed. Database not 
>> included in the backup image."
>> Inconsistent use of ticks in these messages.  Both `, ', and no ticks
>> around %-.64s.
> 
> Ok, style issue. Will fix.
> 
> ...
> 
>>> +Obj *get_db_grant(const String *grantee,
>>> +                  const String *db_name)
>>> +{
>>> +  String priv_type;
>>> +  priv_type.length(0);
>>> +
>>> +  return new DbGrantObj(grantee, db_name, &priv_type);
>>> +}
>> Why are you using a dummy priv_type here?
> 
> Because on restore we do not have the priv_type only the SQL command
> (serialization string). But on backup we need it to build the serialization
> string.
> 
> ...
> 
>>> +  /*
>>> +    String ' from strings.
>>> +  */
>>> +  cptr= ptr + 1;
>>> +  tics= 0;
>>> +  if (strncmp(cptr, "'", 1) == 0)
>>> +  {
>>> +    cptr++;
>>> +    len--;
>>> +  }
>>> +  if (strncmp(cptr+len-1, "'", 1) == 0)
>>> +    tics++;
>>> +  host->append(cptr, len - tics);
>>> +  return 0;
>>> +}
>> Are there any encoding issues here, or is user name always ascii?  I
>> guess the check for tics is not safe if there may be multibyte
>> characters in user names.
> 
> I had not thought of that. I will need to investigate.
> 
> Chuck
> 
> 


-- 
Øystein Grøvlen, Senior Staff Engineer
Sun Microsystems, Database Group
Trondheim, Norway