Hi Chuck,
Thanks for the careful review and for positive feedback :) I have fixed the
problems you indicated. The new patch is here:
<http://lists.mysql.com/commits/82628>
Charles Bell wrote:
> STATUS
> ------
> Patch approved pending:
> - required changes made (they're only minor things)
> - Paul accepts the format as adhering to his requirements
>
> REQUIRED
> --------
> 1. Please fix minor spelling and format issues
> 2. Please explain why in the comments that the one method was renamed.
>
> COMMENTARY
> ----------
> Wow! That was a lot of work. It enhances our documentation very much.
> Thanks, Rafal! Nice drawings too.
>
> DETAILS
> -------
>> Bug #40587 - Doxygen documentation of backup image format is not
>> up to date
>> This patch updates the doxygen documentation for the
>> backup image format
>> and fixes/clarifies issues found there. The format of the
>> documentation is
>> changed to the one which was proposed by Paul for the internals
>> manual.
>> @ sql/backup/kernel.cc
>> - Add the main documentation page.
>> - Include description of how to use kerenel API into doxygen
>> docs.
>
> [1] Please correct spelling. 'kernel'
>
Fixed.
>> @ sql/backup/stream.cc
>> - Add page describing general backup stream format used.
>> Backup image format
>> is described on a separate page.
>> @ sql/backup/stream_v1.c
>> Add pages describing general structure of image format v1 and
>> the image layer
>> format as implemented by functions in this file. Format of the
>> image layer documentation has been changed to the one proposed
>> by Paul for the internals
>> manual.
>
> [2] Please explain method rename.
>
Added this comment:
"Also renamed bstream_rd_image_info() -> bstream_rd_snapshot_info() to follow
new terminology and make it consistent with bstream_wr_snapshot_info()."
> ...
>
>> @ sql/backup/stream_v1_transport.c
>> Added pages documenting the transport layer format of a backup
>> image. Documentation format is changed to the one proposed by
>> Paul for internals
>> manual.
>>
>> modified:
>> sql/backup/kernel.cc
>> sql/backup/stream.cc
>> sql/backup/stream_v1.c
>> sql/backup/stream_v1_transport.c
>> === modified file 'sql/backup/kernel.cc'
>> --- a/sql/backup/kernel.cc 2009-08-11 11:09:31 +0000
>> +++ b/sql/backup/kernel.cc 2009-08-21 11:16:45 +0000
>> @@ -3,6 +3,54 @@
>>
>> @brief Implementation of the backup kernel API.
>>
>> + @todo Use internal table name representation when passing tables to
>> + backup/restore drivers.
>> + @todo Handle other types of meta-data in Backup_info methods.
>> + @todo Handle item dependencies when adding new items.
>> + @todo Handle other kinds of backup locations (far future).
>> +*/
>> +
>> +/**
>> + @mainpage + + @section Structure Structure of the Online Backup
>> System + + @verbatim
>> +
>> + | Online Backup Module
>> + | + (1) | +---------------+
>> +------------------------+
>> + Server ---> | Backup Kernel | <--> | Backup/Restore drivers |
>> + | +---------------+ (3) +------------------------+
>> + Core <--- | Stream layer |
>> + (2) | +---------------+
>> + | (4) ^ (5)
>> + | | |
>> + +--------|---|----------------------------
>> + | v
>> + | Backup Stream Library
>> + | + @endverbatim
>> + + Backup stream library is autonomous so that external
>> applications can link + against it to be able to read backup images
>> created by the system. + The format of backup images is described
>> @ref stream_format "here".
>> + + The components of the system communicate with each other using
>> well defined + interfaces:
>> + -# @ref KernelAPI "Backup Kernel API"
>
> [1] Do you mean, 'Kernel API'?
>
Hmm, I don't see what is wrong. I mean "Backup Kernel API" and this is what is
written...
> ...
>
>> + + @c Version is stored in the following two bytes, the least +
>> significant byte comming first.
>
> [1] coming
>
Fixed.
>> + + @c Backup_image that follows @c backup_prefix is stored in the
>> format + indicated by the version number. Currently only one backup
>> image format + is used -- @ref image_format1.
>> +
>> + The stream format does not provide direct support for compression
>> or + encryption. But a complete backup stream can be compressed
>> and/or encrypted + externally.
>
> [1] space between paragraph?
>
Fixed (made one paragraph).
>> + In that case, it will be wrapped in the compression/encryption
>> format used, + with additional headers added in front of it. Before
>> processing such a stream,
>> + it should be first uncomressed/decrypted and then the 10 byte
>> backup prefix
>
> [1] uncompressed
>
Fixed.
> ...
>
>
>> === modified file 'sql/backup/stream_v1.c'
>> --- a/sql/backup/stream_v1.c 2009-06-30 20:37:35 +0000
>> +++ b/sql/backup/stream_v1.c 2009-08-21 11:16:45 +0000
>
> ...
>
>> + @c Preamble describes the image and objects that it contains. @c
>> Table_data + chunks contain table contents. + @c Summary contains
>> information which is known only when backup image has + been created.
>> Currently this is mainly inforamtion about the validity point of
>
> [1] information
>
Fixed.
> ...
>
>> @@ -348,29 +388,59 @@ int bstream_rd_summary(backup_stream *s,
>>
>> *************************************************************************/
>>
>>
>> int bstream_wr_snapshot_info(backup_stream*, struct
>> st_bstream_snapshot_info*);
>> -int bstream_rd_image_info(backup_stream*, struct
>> st_bstream_snapshot_info*);
>> +int bstream_rd_snapshot_info(backup_stream*, struct
>> st_bstream_snapshot_info*);
>
> [2] Why was this changed?
>
Long time ago there was change of terminology. What is now called "table data
snapshot" was before "table data image". Apparently, I forgot to rename this
function accordingly. Also, the name should be consistent with the name of the
write function:
bstream_wr_snapshot_info(...)
bstream_rd_snapshot_info(...)
> ...
>
>>
>> /**
>> - @page stream_format
>> -
>> - @section header Header
>> + @page image_layer
>> + @subsection header Header Format
>>
>> - @verbatim
>> +@verbatim
>> + header: flags creation_time snapshot_count server_version extra_data
>> +@endverbatim
>> +
>> + @c Header chunk contains the following fields:
>> + - @c flags (2 bytes). Integer image flags.
>> + - @c creation_time (6 bytes). Time of image creation.
>> + - @c snapshot_count (1 byte). Number of snapshots included in the
>> image.
>> + - @c server_version (variable length). Version of the server which
>> created the image.
>> + - @c extra_data (variable length). The remaining bytes to the end
>> of the chunk are reserved
>> + for storing additional information - currently they are ignored.
>> +
>> + Bits in @c flags field have the following meanings:
>> + - Bit 0: Reserved.
>> + - Bit 1 (BSTREAM_FLAG_BIG_ENDIAN)
>> + - If set, the server that created the backup has big-endian
>> architecture.
>> + - If clear, the server has little-endian architecture. + - Bit 2
>> (BSTREAM_FLAG_BINLOG)
>> + - If set, the binlog_coords and binlog_group_coords fields in the
>> summary contain valid values.
>> + - If clear, binlog_coords and binlog_group_coords should be
>> ignored (they are present in the summary but the values are not useful).
>
> [1] Long line, please split.
>
Fixed.
> ...
>
>> + item_entry: type flags catalog_pos [extra_data] [object_metadata]
>> +@endverbatim
>> +
>> + - @c type (2 bytes): Integer object type.
>> + - @c flags (1 byte): Integer flags.
>> + - @c catalog_pos (variable length): Catalog coordinates of the
>> object - exact + format depends on the type of the object.
>> + - @c extra_data (variable length): Optional; present only if +
>> BSTREAM_FLAG_HAS_EXTRA_DATA is set in the flags. (Currently not used.) If
>> + present it consists of
>> + - Integer data length (2 bytes).
>> + - Data bytes, as many as specified by the length.
>> + - @c object_metadata (variable length): String with object's
>> serialization. + Optional; present only if
>> BSTREAM_FLAG_HAS_CREATE_STMT is set in the flags.
>> +
>> + Allowable object type values are given in @ref basic_data_itype.
>> The format of
>> + the catalog coordinates depends on the object type, as described
>> below. + + Object metadata is a serialization of the object as a
>> string, as + provided by server's object services API. Usually, it is
>> an SQL CREATE + statement for the object, but it can also contain
>> additional SQL statements + and other information. + There is also a
>> possibility of storing arbitrary additional metadata in the +
>> optional @c extra_data field, however this field is not used currently.
>
>
> [1] Space between paragraphs?
>
Fixed (it is single paragraph).
> ...
>
>> + @note
>> + It is possible to have external data at the beginning of the first
>> block of + a backup image, This is for situations where, e.g., backup
>> image storage + requires storing additional headers and still wants
>> to preserve block + aligmnet of all the data. For example, when
>> backup image is stored in a file
>> + then it is preceded by a 10 byte prefix containing magic number and
>> image + version number (see @ref stream_format). This 10 byte prefix
>> is stored at + the beginning of the first block and is not considered
>> a part of the backup
>> + image. The @c block_size field of the first block starts at byte 11
>> in that case + and the size of @c block_data equals block_size-15. +
>> If such external prefix is present in the first block, its presence is
>> not + indicated in the image itself. It must be determined by other
>> means (e.g., by + a convention as in the above example).
>> +*/
>
> [1] Space between paragraphs?
>
Fixed (it is a single paragraph).
> ...
>
> Chuck
