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

[Rafal Somla]

Hi Ingo,

Now, when I'm reviewing your backup client code, I found time to read this email 
thoroughly. See some of my comments below. Sorry for such a late reply.

Ingo Strüwing wrote:
> Hi Øystein,
> 
> thank you very much for the first part of a very thorough review.
> Please find answers below.
> 
> Øystein Grøvlen, 11.12.2008 14:24:
>> Ingo,
>>
>> Thanks, for this thorough and well tested patch.  I have not been
>> through all of it, yet, but here are the comments and questions I have
>> so far:
>>
...
>>  * Some questions from playing a bit with the client:
>>
>>      - 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...


>>      - Finish time is always 0.  Why?  It is set correctly in the
>>        backup_history table for the same backup.
> 
> I do not know. It is the value that the stream library provides. I guess
> it is not written to the backup file, or the stream library does not
> fill this element of the "header" when reading the image.
> 

This is a bug in backup kernel which does not write that information to the 
backup image (although it reports it in backup logs). I think it is already 
fixed with one of my patches but not sure if the patch has been pushed already.

...
>>  * Took me some time to understand the role of the bcat_ functions.  I
>>    think this should be explained a bit better.  Do the implementation
>>    have to be this separate for the client and the server, or would it
>>    be possible to share some more code?
> 
> It took me some time too. To say the least. These functions are defined
> by the stream library interface. I had no influence on their architecture.
> 

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

> I must admit that I didn't spend much time in analyzing how to share
> code with the server. Some small experiments required me to include most
> of the server code, which didn't look right. I would also need to learn,
> how the catalog is constructed in the server. A quick look shows that a
> couple of classes are involved. I usually have a hard time to follow all
> the classes used in the backup code. Since not even Rafal suggested to
> do it, I didn't even try.

No, the backup code is not designed to be used by others. If we want to reuse 
it, some more design thinking and refactoring would be required (define public 
interfaces!)

>>  * I would prefer if the addition of error injection to mysys routines
>>    was done as a separate patch.  This is a general mechanism and I
>>    feel that is not related to backup.  I also think other people than
>>    members of the backup team should review this.
> 
> I can't imagine, how this could be done. Such a task wouldn't get
> sufficient priority to be looked at by anyone.
> 
> But I could remove it from the backup client patch. I have tested, what
> I wanted to test with it.
>

I opt for leaving it there.

...
>> Some more detailed questions/comments inline:
>>
>> ...
>>
>>> === modified file '.bzrignore'
>>> --- a/.bzrignore    2008-10-29 14:20:12 +0000
>>> +++ b/.bzrignore    2008-11-25 19:33:34 +0000
>>> @@ -314,6 +314,10 @@ client/mysql_upgrade
>>>  client/mysqladmin
>>>  client/mysqladmin.c
>>>  client/mysqladmin.cpp
>>> +client/mysqlbackup
>>> +client/stream_v1.h
>>> +client/stream_v1_services.h
>>> +mysql-test/mysqlbackup-test.bak
>> Why are the three last entries added?
> 
> I want bzr to ignore these files when running gcommit.
> 
> client/stream_*.h are symbolic links to files in sql/backup. They are
> required for #include directives. The Makefile for the client directory
> does not use an -I../sql option, so I didn't add an -I../sql/backup
> option either.
> 

I really think that backup stream library should be used as a library. However, 
it means it needs to be compiled early in the build process and public headers 
need to be made available in some common directory. All this requires changes 
outside the sql/backup/ i.e., outside our domain. This is why it was not done 
yet and I think it requires a separate task. Until then, I'd consider Ingo's 
solution temporary - ultimately we should not link the library sources or 
headers here.

...
>>> +/**
>>> +  Read from the stream/image.
>>> +
>>> +  @param[in,out]    strm        stream handle, updating position
>>> +  @param[in,out]    data        data container, updating contents and
>> ptrs
>>
>> I do not think it is easy to understand from this comment what is
>> producer and comsumer here.  And what "updating contents and ptrs"
>> implies.
> 
> Agree. I tried to keep the parameter descriptions short. This didn't
> work out well.
> 
> Please note that this is also a callback for the stream library. It's
> signature is specified in stream_v1_services.h:
> 
> +  /**
> +    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?

=== modified file 'sql/backup/stream_v1_services.h'
--- sql/backup/stream_v1_services.h     2007-11-29 19:58:12 +0000
+++ sql/backup/stream_v1_services.h     2008-12-19 13:06:09 +0000
@@ -187,16 +187,27 @@ int bcat_create_item(struct st_bstream_i
  /**
    Function writing bytes to the underlying media.

-  For specification, see @c bstream_write_part() function.
+  Writes bytes stored in the specified fragment of the buffer, the same as is
+  done in @c bstream_write_part() function. See documentation of that function
+  for details.
+
+  @retval BSTREAM_OK at least one byte was written (or copied to the output buffer)
+  @retval BSTREAM_ERROR  error was detected
  */
-typedef int (*as_write_m)(void*, bstream_blob*, bstream_blob);
+typedef int (*as_write_m)(void *s, bstream_blob *data, bstream_blob buf);

  /**
    Function reading bytes from the underlying media.

-  For specification, see @c bstream_read_part() function.
+  The function stores bytes in the specified fragment of the buffer, the same
+  as is done in @c bstream_read_part() function. See documentation of that
+  function for details.
+
+  @retval BSTREAM_EOS  end of stream was detected while reading data
+  @retval BSTREAM_ERROR error was detected while reading data
+  @retval BSTREAM_OK   successful read - at least one byte was read
  */
-typedef int (*as_read_m)(void *, bstream_blob*, bstream_blob);
+typedef int (*as_read_m)(void *s, bstream_blob *data, bstream_blob buf);


If yes, could you include it in your patch? If not, what remains unclear?

> Fortunately, at least the function comment of bstream_read_part() tries
> to explain, how "bstream_blob"s are used.
> 

And succeeds? If not, then what is the problem?

> And, there is a reference implementation in stream.cc: stream_read().
> Unfortunately, its comments doesn't add much insight.
> 
> In the end I was happy that I made it work at all. I didn't feel much of
> an obligation to explain it better than what I had to work with.
> 
> But now I see, I was in error. I take the blame. How about this description:
> 
> 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


> 
>>> +  @param            envelope    not used
>> 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!


>>> +static int
>>> +str_open_rd(struct st_stream *strm, const char *path, uint *version_p)
>>> +{
>>> +  MY_STAT               stat_area;
>>> +  size_t                lgt;
>>> +  int                   rc= BSTREAM_OK;
>>> +  int                   errpos= 0;
>>> +  uint16                version;
>>> +  const unsigned char   backup_magic_bytes[8]=
>>> +    {
>>> +      0xE0, // ###.....
>>> +      0xF8, // #####...
>>> +      0x7F, // .#######
>>> +      0x7E, // .######.
>>> +      0x7E, // .######.
>>> +      0x5F, // .#.#####
>>> +      0x0F, // ....####
>>> +      0x03  // ......##
>>> +    };
>> I suggest that these magic bytes are defined a place where the same
>> definition could be used for both server and client.
> 
> Hehe. I tried this with another define. The problem is that the headers
> in sql/backup/ are heavily interwoven with server code.
> 
> The two exceptions stream_v1.h and stream_v1_services.h are not
> appropriate either, because Rafal's architecture is so that the magic
> bytes will never change, while a new version of the stream library may
> change everything. So we would probably need a new header file. I'm
> awaiting your proposal.
> 

Yes the current backup stream library is not a good place to put it there 
because it is not intended to read any type of backup stream, but only a backup 
image in format v1. In fact, contrary to its name which is perhaps ill chosen, 
the library is encapsulating not a backup stream as such but rather the format 
of the backup image, by providing stream interface for reading/writing an image 
in that format.

In the future, there will be more formats around, and more libraries each 
describing a single format (at least that was the idea). Then it would be 
perhaps good to have a top level library, which will be able to read stream, 
detect the format and use correct format-specific library to interpret the rest 
of the stream. Such top-level library would be a proper place to export the 
backup magic bytes.

Note however that these magic bytes are meant to be "written in stone" as for 
example the magic bytes for gzip compressed data.

>>> +/**
>>> +  Create global iterator of a given type.
>>> +
>>> +  Possible iterator types.
>>> +
>>> +  - BSTREAM_IT_CHARSET:    all charsets
>>> +  - BSTREAM_IT_USER:       all users
>>> +  - BSTREAM_IT_TABLESPACE: all tablespaces
>>> +  - BSTREAM_IT_DB:         all databases
>>> +
>>> +  The following types of iterators iterate only over items for which
>>> +  some meta-data should be saved in the image.
>>> +
>>> +  - BSTREAM_IT_GLOBAL:     all global items in create-dependency order
>>> +  - BSTREAM_IT_PERDB:      all per-db items except tables which are
>> enumerated
>>> +                           by a table iterator (see below)
>>> +  - BSTREAM_IT_PERTABLE:   all per-table items in create-dependency
>> orders.
>>> +
>> I do not quite understand this comment.  Are the three latter no
>> possible iterator types since they are in another list?
> 
> I'm not sure either. I copied the comment verbatim from the stream
> library interface definition in stream_v1_services.h.
> 
> I guess that "Possible iterator types" applies to all the iterator
> types, but that the latter three have an additional comment.
>

Yes, that was the intention.

...
>>> +
>>> +  case BSTREAM_IT_DB:
>>> +    iter->it_array= &bup_catalog->cat_databases;
>>> +    DBUG_PRINT("bupstrm", ("database"));
>>> +    break;
>>> +
>>> +    /* purecov: begin inspected */
>>> +  case BSTREAM_IT_TABLESPACE:
>>> +    iter->it_array= &bup_catalog->cat_tablespaces;
>>> +    DBUG_PRINT("bupstrm", ("tablespace"));
>>> +    goto err;
>>> +
>>> +  case BSTREAM_IT_GLOBAL:
>>> +    errm("iterator type not yet implemented: global\n");
>>> +    DBUG_PRINT("bupstrm", ("NOTYET implemented: global"));
>>> +    goto err;
>>> +
>>> +  case BSTREAM_IT_PERDB:
>>> +    errm("iterator type not yet implemented: perdb\n");
>>> +    DBUG_PRINT("bupstrm", ("NOTYET implemented: perdb"));
>>> +    goto err;
>>> +    /* purecov: end */
>> Ditto.
> 
> Well, I don't know why these have not been used during all the tests I
> did. There were definitely tablespaces and views (per-db items) in the
> backup files.
> 
> A vague guess may be that all of these have no sub-objects. So that the
> stream library does not need to access the "parent" structure ever. Only
>  the databases iterator was used. The catalogs contained sub-items of
> databases. When handing over such sub-type to the bcat_add_item()
> function, it sets up the item structure with a reference to the database
> object within the application's catalog. To get at that reference, it
> needs to use an iterator to find the database by its "catalog position".
> 

Yes, it is a good guess. Other kinds of iterators are used only when performing 
restore.

...
>>> +enum enum_bstream_ret_codes
>>> +backup_read_catalog(void* image_handle, struct st_backup_catalog
>> *bup_catalog)
>>> +{
>>> +  struct st_stream *strm= (struct st_stream*) image_handle;
>>> +  enum enum_bstream_ret_codes brc;
>>> +  DBUG_ENTER("backup_read_catalog");
>>> +  DBUG_ASSERT(strm);
>>> +  DBUG_ASSERT(bup_catalog);
>> Would it not be appropriate here and in the other similar _read_
>> functions to assert that stream_pos is as expected?
> 
> - This would not work with the current way, I assign string literals to
> stream_pos (unless we do string comparison). I would at least have to
> use file-global string constants, if not enums as you suggested. Since I
> found that I did a string compare for "meta data", I did now define
> string constants like: const char *STREAM_POS_META_DATA= "meta data". So
> this argument is void now.
> 
> - Assertions are used to catch flaws in the implementation. Wrong stream
> pos is more likely to happen due to data corruption. Then it is better
> to check with 'if'.
> 
> - I guess that this check is done in the stream library. But if you
> think we should check it here anyway, then ok.
> 

No, it is not. But perhaps it should! I did not get that idea when writing the code.

> - It may take some time to figure out allowed combinations. For example,
> I guess that table data do not need to be present. If the summary
> follows the meta data, this should proably be accepted. Summary can be
> inline in the header. Etc.
> 

Yep, there are few combinations. With help from Paul, I hope we will have backup 
image format well documented soon.

...
>>> +/*
>>> +  Catalog items are located by "catalog coordinates". The format of the
>>> +  catalog coordinates depends on the type of item. It is specified as
>>> +  follows:
>>> +
>>> +  [item position (global)]= [db no.]
>>> +  [item position (table)]= [ snap no. ! pos in snapshot's table list ]
>>> +  [item position (other per-db item)]= [ pos in db item list ! db no. ]
>>> +  [item position (per-table item)] = [ pos in table's item list !
>>> +                                       db no. ! table pos ]
>> What does '!' mean.  If it is just a separator, please, choose
>> another character so that people do not believe it has something to do
>> with negation.
> 
> I copied the from stream_v1.c. I hesitate to introduce an inconsistent
> syntax notation here.
> 

Paul is working now on this document: 
http://forge.mysql.com/wiki/MySQL_Internals_MySQL_Backup_Stream_API
Perhaps you could use notation from there. Unfortunately, item coordinates are 
not covered yet (I just sent an email about this).

Rafal