Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2745) WL#4630

[Ingo Strüwing]

Hi Rafal,

Rafal Somla, 19.12.2008 16:20:
...

>    Sorry for such a
> late reply.


Me too. So many other things took priority.

> Ingo Strüwing wrote:

...

>> Øystein Grøvlen, 11.12.2008 14:24:

...

>>>      - What does "extra data length" mean?  And why is it always 0?
>>
>> It is the length of the "extra data" that could be present in the backup
>> image for an object. Currently, we do not write extra data to the backup
>> image. Hence it is always 0.
>>
> 
> Perhaps no information about extra data should be printed if it is not
> present...


I suggest to display empty data with verbose. If suppressing completely,
user might suspect, client is broken.

...
> Does that mean that the documentation provided in stream_v1_service.h is
> not clear enough? Any suggestions how to improve it?


Yes. I would like to see an overview. What is the stream library, how
does it work? What functions are to be called in which order? Which
call-back functions does it call on read and on write? Some words about
"catalog coordinates". What are requirements for the catalog that cannot
be freely chosen by the application?

Full, complete function comments. Mention of which data is allocated by
the stream library, what needs to be copied by the application, and what
needs to be freed by the application.

...
>> +  /**
>> +    Function reading bytes from the underlying media.
>> +    For specification, see @c bstream_read_part() function.
>> +  */
>> +  typedef int (*as_read_m)(void *, bstream_blob*, bstream_blob);
>>
>> However, bstream_read_part() is the caller of the as_read_m typed
>> function. So this comment basically says "Go and figure out yourself,
>> how it is used." That's what I had to do.
>>
> 
> The intention was to say that as_read_m, which has the same parameters
> as bstream_read_part() should also implement the same behaviour (both
> functions are reading bytes from the underlying stream).
> 
> I don't like copying the same documentation in two places because then
> they are bond to diverge eventually.
> 
> Would such change in the documentation explain it better?


Perhaps. At least the added parameter descriptions are valuable. But if
both functions "should also implement the same behaviour", why do we
have two of them at all?

And keeping the documentation somewhere deep inside a file, which an
implementor should normally not need to look at, doesn't seem right.

If you move the main documentation to as_read_m() and point to it from
bstream_read_part() that would be more acceptable IMHO.

...
>> The 'data' parameter is a reference to an st_blob structure, which
>> contains two pointers, 'begin' and 'end'. When the function is called,
>> the pointers point into a buffer to receive the data from the stream.
>> They are to be placed starting at 'begin' and added up to 'end'. On
>> return of the function, the 'end' pointer is adjusted towards 'begin',
>> if less than the requested amount of data was read from the stream.
> 
> I think it is 'begin' pointer which is adjusted towards 'end':
> 
>   buf  [---------------=================--------]
> 
>   data blob before reading:
> 
>                       [=================]
> 
>   data blob after reading:
> 
>                        **********[======]
>                        ^          ^
>                        |          space remaining free
>                        bytes which have been read
> 


Unfortunately, we seem to have a completely different view of things.

While you may know, where begin and end of the different buffer objects
point to, I can just guess.

If I am right, that in the middle row, we have data.begin at the first
'=' and data.end at ']', and in the lower row data.begin at the first
'*' and data.end at '[', then, in my view, data.end references a lower
memory address as before the call, while data.begin stayed the same.
This is what I call "the 'end' pointer is adjusted towards 'begin'".

To release me from guessing, and give me some certainty, I suggest to
change the chart similar to this (or a correct version thereof):

  buf  [---------------=================--------]
        ^                                       ^
        envelope.begin                          envelope.end

  data blob before reading:

                      [=================]
                       ^                ^
                       data.begin       data.end

  data blob after reading:

                       data.begin
                       |         data.end
                       v         v
                       **********[======]
                       ^          ^
                       |          space remaining free
                       bytes which have been read


...
>>> What is the intention of envelope?
>>
>> From the reference implementation and the calling function, I guess it
>> is the buffer as it has been allocated and into which the 'data'
>> pointers point. It is not necessary that 'data->begin' is at
>> 'envelope->begin', nor that 'data->end' is at 'envelope->end'. But both
>> 'data' pointers must be between the 'envelope' pointers.
>>
>> However, the parameter is never used. Not in my implementation, nor in
>> the reference implementation. I don't even have an idea, what it could
>> be useful for.
>>
> 
> If I remember correctly the intention was to not exclude a following,
> very remote I agree, use scenario:
> 
> Suppose that the underlying stream is organized so that the proper
> content is wrapped in some kind of headers/footers. For example
> checksums or something like this. Having such interface to the stream
> read function allows the implementation to use the provided buffer to
> store the header/footer data if there is enough space for it. So the
> implementation could work like this:
> 
> If next thing in the stream is N-bytes long header followed by data then
> do the following:
>  1. See if there is N-bytes available between beginning of the buffer and
>     start of the area where data should be stored.
>  2. If yes, then save these N-bytes on a side.
>  3. Read into the buffer (the envelope) the header followed with as much
> data as
>     needed (and can fit) aligned so that data falls in the correct place.
>  4. Do header processing.
>  5. Restore the N-bytes overwriting the header.
> 
> This way implementation can use the provided buffer for low-level reads
> and avoid copying big amounts of data (as compared to the presumably
> short header/footer).
> 
> I'm not going to defend this design or argue that we need this level of
> generality. I designed it like this because I like general designs.
> Later it was not refused by reviewers and... here it is!


I do not aim at changing the design. If I had a wish free, I'd wish that
parts of the above is included in the function documentation.

The design is a bit unusual for a cache. So it deserves more
documentation as usual.

...


Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028