Oystein,
Answering interdelinia...
> 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?
...
> > +--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.
> > +--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.
...
> > +--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.
...
> > +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
