Hi Chuck,
this message may be a little like hair splitting. Nothing herein
contains a requirement to you or the patch. I merely write this in the
hope you may better understand me. And perhaps provide the opportunity
for you to make me better understand you.
Charles Bell, 25.11.2009 23:25:
> Ingo,
>
> Here are my more detailed responses and the new patch.
IMHO it is not a good move to split responses to my review into three
emails (two directly replying to my review, one commit email with
changes). This is likely to produce chaos.
I have to respond to each of the emails. I fear, one or more may not be
noticed.
The email, I respond to herewith, contains duplicates of replies, I have
received in the former email already. Unfortunately, most are not
textual copies, but contain slightly different thoughts. So I feel the
need to reply again, probably repeating myself, and if not, it might go
undetected as I cannot expect you to read all once again. Anyway, let me
try.
...
>>> 3) The backup system must be changed to allow users to adjust the
>>> privilege behavior of MySQL Backup by allowing them to
>>> turn off backup elevation, restore elevation, and restore
>>> prechecking using startup options and/or variables. This patch
>>> implements startup options and global + session variables to do
>>> the following:
...
>> 1) This is now inaccurate. Please specify variable type per option.
>
> This has been changed to:
...
> backup_elevation - startup option, global read only variable
...
> restore_elevation - startup option, global read only variable
...
> restore_precheck - startup option, global and session variable
...
Good.
...
>> 2) A plea:
>>
>> I often see "REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost'", that
>> is, revoke all global privileges. This sometimes happens even after
>> "CREATE USER". Just to be sure.
>
> What does 'Just to be sure' mean?
Doing something, which is not required, but serves only the purpose to
make oneself sure of something.
When I leave home, I turn back to check if I locked the door, just to be
sure. A hunter, who shot a wild pig shoots again in its head, just to be
sure it is dead. A C programmer initializes global variables as zero,
just to be sure the compiler does not set a different value against its
specification. A tester revokes privileges from freshly created user,
just to be sure that he does not have privileges.
> To satisfy this and similar complaints, I now revoke the _exact_
> privileges rather than a blanket revoke.
Good. That's one way to solve it. But the disadvantage is that we need
to know, what the previous privileges have been. If one test case
adds/removes privileges, the successor must be changed.
I think that the idea to start from scratch every time is good. This
could be done by DROP USER + CREATE USER *or* REVOKE ALL PRIVILEGES,
GRANT OPTION FROM user.
But combining CREATE USER with an arbitrary set of revokes does not look
good.
Anyway, the quoted sentence does not contain a change request. I just
wanted to say that the test contain unnecessary things, which you feel
better with, and thus hope, you would accept another unnecessary thing,
which *I* feel better with: SHOW VARIABLES LIKE 'restore_%' before every
RESTORE.
...
>> I often see "FLUSH PRIVILEGES", even though no updates on the privilege
>> tables are done (GRANT/REVOKE don't need it). Just to be sure.
>
> I was told always use FLUSH PRIVILEGES (despite what the documentation
> says) because it is 'good form'. If you disagree, then you can omit it
> as you see fit but I choose to leave them in.
We had that discussion before. I accept that you feel better with it.
The quoted sentence does not contain a change request. I just wanted to
say that the test contain unnecessary things, which you feel better
with, and thus hope, you would accept another unnecessary thing, which
*I* feel better with: SHOW VARIABLES LIKE 'restore_%' before every RESTORE.
>
>> But I see "SHOW VARIABLES like 'restore_%'" pretty seldom. Since it is
>> less easy to calculate their values, based on former settings, or
>> presence/absence of a -master.opt file, or implicit default values, than
>> it is to calculate the grants, I ask for a "SHOW VARIABLES like
>> 'restore_%'" before every RESTORE. Just to be sure.
>
> Yes, it is seldom because they're global read only variables so we don't
> need to check their values. However, I will add one SELECT for each of
> these at the start of each test.
Thank you. You do my a great favor. I am not so safe in knowing, when
which variable has which value.
I just hope, you put them right before the RESTORE, after the new
connection has been opened. Or show the global values. Otherwise they
would show the session value of the root connection, which is of no
interest.
...
>> 3) Do we need privileges on mysql.* for RESTORE? If yes, do we need more
>> than SELECT?
>
> Once again, I was asked to do it this way to ensure we have all
> privileges covered. My own explanations as to what the minimal set of
> privileges was rejected. Hence we compromised and added all privileges
> on mysql database. If you severely object you can open a bug but I will
> leave it like this in the patch.
Ok. I will no longer complain. But I will tell you a short story. Today
I had lunch with my parents. They noticed that I was brooding over
something. I explained that was reviewing a colleagues work. That it was
not wrong, but I was not happy about it. I asked them to imagine, they
would have bought new curtains, including installation. When the
craftsman finished, the curtains looked great. They had the ordered
color and were wide enough to cover the windows. But then they would
notice that one piece ended two centimeters above the floor. One piece
ended five centimeters above the floor. One piece draggled on the floor.
"Would you be happy with such work?". They did understand.
I am not sure if I need to point out that different lengths of curtains
correspond with different sets of privileges, granted to mysql.* in the
different test cases.
...
> --echo #
> --echo # Test Case 5 : Show the TRIGGER_ACL privilege is needed for
> --echo # restore when restore_elevation is OFF and
> --echo # restore_precheck is ON.
> --echo #
>
> I also add a SELECT of the variables for clarification. In this case, I
> am going with your (I think) suggestion to be safe and display them
> where it matters for clarity.
Sounds good so far. I'll look at the new test case it total.
...
>> 9) Here is still one of these "comparisons", which I don't understand.
>> If we would "Show list of all objects in the database" after the
>> previous RESTORE too, then I'd get the idea.
>>
>> See also my my email of Fri, 20 Nov 2009 14:10:49 +0100.
>
> Removed.
>
> General comment, Ingo: I think I miss some of these because when I read
> the review comments it isn't clear to me that there is more than one of
> these types of things. Please in the future help me out by stating
> something to that effect. You need not repetitively cite the issue, but
> a gentle nudge/reminder like 'This occurs elsewhere' or something would
> be very helpful. In a large patch it is sometimes hard to be thorough.
I am sorry, if I forgot it this time. Usually I do make such note.
With respect to "3) Again, do we need privileges on mysql.* for
RESTORE?", I repeated it everywhere as I asked that question twice
before, but had not seen an answer. As you can see in my former email, I
did meanwhile investigate this myself.
...
>> 11) Sad that the test ends here. Sad that my suggestion from my email of
>> Fri, 20 Nov 2009 14:10:49 +0100 are not followed.
>>
>> The test does not show that RESTORE fails during metadata execution
>> stage. At least not to someone with little knowledge of the code. The
>> error message "Access denied" could come from everywhere.
>
> I strongly disagree. It does for the reasons I stated. It is failing
> during the metadata CREATE stage.
Confirmed by debugging. So do we have a new rule, that it is fine to
write test cases so that their correctness does not need to be
verifyable other than by debugging?
>
>> My suggestion was to add CREATE, DROP to the privileges. Then RESTORE
>> will fail with ER_BACKUP_CANT_RESTORE_SROUT instead of
>> ER_BACKUP_CANT_RESTORE_DB. And another SHOW FULL TABLES, SELECT events,
>> etc after switching back to root will show that only t1 and t2 exist
>> and all other objects have been destroyed. This makes it very obvious
>> that RESTORE "failed in the middle".
>
> To you perhaps it makes more sense but not to me as I know it is failing
> in the middle. Besides, users are not the general audience for these
> tests -- we are.
If "we" includes me, then I can keep up my complaint. I need debugging
to verify that the test case works as described.
> If we wanted better feedback to the users to indicate
> it is failing in the middle I might suggest we open a bug to request
> that the error messages be made more user friendly.
You mean "RESTORE failed in the middle" or such? Strange. But how about
a warning "some objects have been deleted", if at least one DROP had
been successful? And yes, it is not a change request to this bug fix.
Just an idea for a feature request.
>
> Regardless, I have added such a test case step to satisfy your concerns.
Thank you. I appreciate it.
...
>> 14) This looks a little pointless. From test file start, we know that
>> the global value is ON already. It would give us more, not to set the
>> local value before showing, which value the new connection did inherit.
>
> Ok.
>
>> And if setting the local value, perhaps to show, that it does indeed
>> change, then please set the inverse value and check it. Otherwise we
>> have nothing but the absence of an error message.
>
> I am sorry but I do not understand this. I have changed the test case as
> I interpret these paragraphs and hope it is better. If not, you will
> need to explain it a bit more, please.
What the second paragraph means is this: I have seen these lines:
+# Show root user cannot change backup_elevation and restore_elevation
+# but can change restore_precheck.
+#
+SET @@global.backup_elevation = ON;
+ERROR HY000: Variable 'backup_elevation' is a read only variable
+SET @@global.restore_elevation = ON;
+ERROR HY000: Variable 'restore_elevation' is a read only variable
+SET @@global.restore_precheck = ON;
+SET @@global.restore_precheck = OFF;
+#
By the existence of the error messages, we see that we cannot change the
variable value for backup_elevation and restore_elevation.
For restore_precheck we see that no error happens. But we do not show
the value after the change. Hence we conclude from the non-existence of
error messages that the change was successful.
I just wanted to say that a SHOW of the changed values would give a
little more confidence in the correct behavior of the variables.
However, I did not ask to add such show. In contrast, I complained that
you show the value after you did not change the value (or, more
accurate, after you changed the value to what it already was before).
It is the imbalance of thoroughness, which makes me feel uncomfortable.
Checking, where it is pointless, not checking where it would add value.
But again, it is not that important that I would block the push.
...
>>> +--echo # Test Case 3 : Show that SUPER is needed to complete restore
...
>>> +GRANT ALL ON mysql.* TO 'bup_some_priv'@'localhost';
...
>> 3) Again, do we need privileges on mysql.* for RESTORE?
>
> Again, left in due to unknown privileges needed on mysql database.
...
...
>>> +--echo # Test Case 4 : Ensure that a user can restore the database
...
>>> +GRANT SELECT ON mysql.* TO 'bup_some_priv'@'localhost';
...
>> 3) Again, do we need privileges on mysql.* for RESTORE?
>
> Again, left in due to unknown privileges needed to mysql database.
Here we see it on a single glance that the tests are inconsistent.
First, you say, you want to leave GRANT ALL due to unknown privileges
needed, then you want to leave GRANT SELECT due to unknown privileges
needed.
If you don't know, which privileges are needed whatsoever, why don't you
grant ALL everywhere?
However, since all RESTOREs, that have SELECT only, work, it would be
more logical to me, just to grant SELECT everywhere. But this is a
weaker wish. More important to me are consistent tests. If you want ALL
everywhere, so be it.
...
>>> +/* Copyright (C) 2009 MySQL AB, 2009 Sun Microsystems, Inc.
>>
>>
>> 15) IMHO the "2009 MySQL AB, " should go away.
>> See https://inside.mysql.com/wiki/LegalInfoCopyright
>> "
>> Code newly developed since Oct 1 2008 should use only a Sun Microsystems
>> copyright notice as follows:
>>
>> Copyright 2008 Sun Microsystems, Inc.
>> "
>
> Ok. This is extremely confusing because I see all manner of copyright
> statements. I will use what you request and dare others to challenge.
Thank you. I quoted the official Wiki page. But there is also an
amateurish explanation: Since Oct 1 2008, Sun Microsystems, Inc. is the
sole copyright owner of all new code. Every new file created after that
date belongs only to Sun. It would be wrong to claim, MySQL AB shares
the copyright. Files existing before that date contain code, which was
written for MySQL AB. Hence MySQL AB has the copyright on that code. If
a file contains code, which was present before that date, and code,
written later, the both have copyright on parts of the file.
If a new file is created, and code moved over from an old file, things
might become complicated. The Wiki page doesn't comment on this case.
...
> For the sake of clarity I change the wording to this:
>
> It begins by checking that the user has RESTORE for each database listed
> in the backup image. If the user does not have RESTORE for any database
> in the list, an error is generated. Next, specific object-level
> privilege checking is performed for all of the objects in the database.
> The object-level privileges checked include the following.
>
> I honestly don't know how I can make this any clearer.
Ok, I give up. English is pretty foreign to me. Can you just please be
so kind to tell me what this sentence would mean:
"If the user does not have RESTORE for all databases in the list, an
error is generated."
Would that mean, the error does not happen, if a single permission
exists? Or would it be the same as before, that is, 'any' and 'all' are
equivalent?
And then please look at these:
1. +----------+ 2. +----------------------+
| building | | building |
| | | |
| door | | |
| +====+ | | door door door |
| | | | +-+====+-+====+-+====+-+
| |door| |
| +====+ |
| | | |
| |door| |
+--+====+--+
A) "The visitor cannot enter the building if he does not have a key to
any door."
B) "The visitor cannot enter the building if he does not have a key to
all doors."
Which sentence matches which picture?
...
>> 19) Did I already state my personal disagreement for the records? I
>> yield to the team decision. But I don't agree that we need to copy the
>> replication behavior in such detail that we keep a one-line function
>> equal to the equivalent one-line function used by replication. We do not
>> use the same function as replication uses as we need to do small
>> changes. So it would be no problem to exclude privileges, which
>> BACKUP/RESTORE *never* will need, just because replication might need
>> them in its own function.
>>
>
> Understood. Do you expect something from me on this?
No.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
