Hello Marc,
Marc Alff a écrit, Le 31.07.2009 04:45:
> Hi Guilhem,
>
> Guilhem Bichot wrote:
>> Hello,
>>
>> Marc Alff a écrit, Le 30.07.2009 18:33:
>>> #At file:///home/malff/BZR-TREE/mysql-azalea-perfschema/ based on
>>> revid:marc.alff@stripped
>>>
>>> 2841 Marc Alff 2009-07-30
>>> Revised the previous fix in the file io instrumentation,
>>> fixed the code interpreting the return value of
>>> my_read/my_write/etc.
>>> Minor cleanup in pfs_atomic.
>> thanks for the fast fixes. One point below.
>>
>>> === modified file 'include/mysql/psi/mysql_file.h'
>>> --- a/include/mysql/psi/mysql_file.h 2009-07-30 15:11:22 +0000
>>> +++ b/include/mysql/psi/mysql_file.h 2009-07-30 16:33:11 +0000
>>> @@ -761,12 +761,10 @@ inline_mysql_file_fread(
>>> if (locker)
>>> {
>>> size_t bytes_read;
>>> - if (result)
>>> - /* my_fread does not always return the number of bytes read. */
>>> - bytes_read= (flags & (MY_NABP | MY_FNABP)) ? count : (size_t)
>>> result;
>>> + if (flags & (MY_NABP | MY_FNABP))
>>> + bytes_read= (result == 0) ? count : 0;
>>> else
>>> - /* reporting 0 in case of error */
>>> - bytes_read= 0;
>>> + bytes_read= (result > 0) ? (size_t) result : 0;
>> "result" and "bytes_read" are both size_t so the cast is not needed.
>> "size_t" can be unsigned (on my Linux debug build it is, if I inspect
>> "mysqld" with gdb, "ptype size_t" says e long unsigned int). In case of
>> error my_fread() may return (size_t)(-1) (even if MY_NABP and MY_FNABP
>> are not used); so "result" will be a huge positive number, and
>> "result>0" will be true, and the huge positive number will be reported
>> in P_S tables, when you wanted 0. Given that all mysys functions
>> involved claim to return -1 if error, you could test for result!=-1
>> instead of result>0.
>>
>
> Thanks, this has now been addressed.
>
> As far as the *code* in psi/mysql_file.h is concerned, the code seem to
> be (too slowly) converging, so that is nevertheless still progress.
yes, to me this piece is perfect now.
> As far as the *review* is concerned, I don't even want to claim that
> this code has been definitively 'fixed' since it is already the third
> pass on it, but I really want to point out that the root cause of all
> this mess is the IMO unacceptable design/documentation of my_read(), and
> does not lie in the implementation of psi/mysql_file.h alone, no matter
> how broken it may be.
>
> The way I see my_read() and friends:
>
> When an API returns sometime -1 and sometime 0 to signal an error, it is
> broken.
>
> When an API returning a >0 value means sometime an error and some other
> time success, it is broken.
>
> When an API documentation mention sometimes '-1' sometimes '(size_t)-1'
> as a return value, it is broken, as this fools the reader into using 'if
> (result > 0)' which of course does not work when size_t is unsigned (as
> you have found).
>
> When an API documentation mention '(size_t)-1' instead of mentioning
> MY_FILE_ERROR, which is the return value supposed to be used in the
> first place, it is broken.
>
> When an API does not document the effect of every input parameter, for
> example MY_FULL_IO, it is broken.
>
> When an API does not document the effect of every combination of input
> parameters, for example the precedence between MY_NABP/MY_FNABP and
> MY_FULL_IO, it is broken.
I agree with some points above and disagree with some others (for
example the distinction between (size_t)-1 and MY_FILE_ERROR: the second
is defined to be the first), but it does not really matter.
> My whole point is: in all *fairness*, if my_read() was better designed
> and documented in the first place, maybe you would not have to review
> psi/mysql_file.h so many times, and I would not have to fix it so many
> times, but this is not what I am reading from your comments.
You may be referring to my comment where I said that my_read() has no
bugs and the problem is that mysql_file.h wrongly assumed that the
returned value is always a number of bytes.
I confirm my comment, with the precision that maybe we don't have the
same interpretation of "bug". For me "bug" is a problem which a MySQL
user could experience: where I can make a testcase (usually in SQL or
with the C API), and run it, and the system will return bad data, crash,
be too slow. Maybe for you "bug" can also mean "badly designed code",
"confusing code", "badly documented code" (and then it's a bit subjective).
I can certainly agree that my_read() is hard to understand, that it
takes quite some energy to find out what it does in all cases (probably
a multi-hour task), and the behaviour has so many if() that one cannot
keep it in mind and has to rediscover it each time. There is largely
room for improvement there (Reengineering?). Surely, if my_read() was
simpler we would have made less round-trips review.
But my_read() shows no known user-level bug, so I would not say the
following: it's broken thus should be fixed and until it is, Perf Schema
will not be changed and will return wrong data.
It's a theoretical discussion now anyway, as your code works I believe.
--
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com
