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

[Chuck Bell]

Oystein,

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

The drop user thingy:

I have an alternative solution to the anonymous user problem. But first, an
explanation is in order.

I am not certain of the source (there are not scripts or test includes that
do this) but somewhere along the line we end up with the anonymous user
defined in information_schema.*_privileges tables but not actually listed as
a user. So when backup runs and reads the information from
information_schema it is including the grants for the anonymous user. This
causes warnings on restore. This is why I removed the anonymous user before
each backup in the ddl blocker and no data tests.

I think a more correct solution would be to only backup grants for users
that exist at the time of backup. This is easily done since all of the
information is present. If we find users want to include users that do not
exist at backup, we can later add a WITH_ALL_USERS or some such option. But
for now, this is the safest way to backup grants. If you concur, I will
release a new patch with this change (very small).

The encoding issue:

Still digging on this one. It does not appear that multibyte user names are
permitted but I cannot confirm.

Some other comments:

> > Could not restore view ... Please check the view definition 
> for possible
> > missing dependencies.
> > 
> > How is that?
> 
> Looks better to me.

Changed.


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

Corrected.

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

Spoke too fast. You must use quotes if you provide user@host as I have done.
However, I have added bup_user2 using localhost which will demonstrate you
can use the option to not include the quotes.

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

It should be restore -- my mistake.

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

Fixed.

Chuck