Hi Chuck,
The format of the backup image specifies that the 2 first bytes of image's
header contain backup image flags. This is 16 bits storing information about the
image. Currently 3 of these bits have defined meaning (as described in
stream_v1.h). These are:
bit 0: tells the position of the summary section (at the end or in the preamble)
bit 1: tells the endianess of the machine on which the image was created
bit 2: tells if binlog position is stored inside the summary section
Of these 3, bits 0 and 2 are obviously needed to correctly read backup image. We
need to know where the summary section is as otherwise we would confuse it with
other sections of backup image. Bit 2 tells us whether the final bytes of the
summary section (starting from 12-th one) contain a valid binlog position or
should be ignored.
As for bit 1, the information stored here might be useful when using native
backup drivers. We can't guarantee that these drivers produce data snapshot
which is endianess agnostic. Thus bit 1 contains information which might be very
important for the user. Suppose that a user gets a backup image of very
important data and has problems restoring it. By looking at image's header he
can discover that a native backup driver was used to create it. Then he can
examine the endianess flag and try to restore on a computer with the same
architecture. Without the flag he would have to guess that...
In general, the justification for this code is to store in the backup image some
information which can be generally useful for interpreting image's contents.
Even if our code doesn't use all this information right now, we want to store it
there and promise to do so in the backup image format specifications. Given
that, not storing the information is a bug which this patch fixes.
Rafal
Chuck Bell wrote:
> All,
>
> I have a problem with this bug report. There is nothing wrong with
> implementation and the patch is fine, but it is useless. Why are we adding
> these flags if they are not used? Is this a 'missed it last time' repaired
> omission? If that is the case, this work should be delayed until such time
> as the flags are needed. Or simply add documentation concerning how the
> flags should be used but do not actually add the code until it is needed.
>
> I think the bug report should be moved back to open and modified to actually
> accomplish something.
> We should not add code that doesn't do anything or isn't acted on --
> normally it's a bomb waiting to explode. In this case, flags are innocuous
> but the premise remains: bug reports should fix something. I don't see where
> this fixes anything that is broken (see below).
>
> I can make a case for adding code to process one of the flags: Shouldn't the
> code that does the restore interrogate the flags to see if the image
> contains a validity point and if so read it and report that information to
> the user via the log (backup_history)? This, at least, accomplishes
> something that is indeed missing in the code.
>
> As for the endianess (sp?) flag... That smells of something much more
> sinister. Why do we have the flag if we are not checking endianess on
> restore? Is it possible to backup on a little endian and restore on a big
> endian machine (or vice-versa)? If the answer is 'yes' then the flag is
> useless and should not be added. If the answer is 'no' then perhaps this bug
> report should be about that problem and therefore solve it.
>
> I think also that part of the solution to this bug report should correctly
> document the use of the flags.
>
> Until these questions are answered and/or someone convinces me these changes
> are needed even though they accomplish nothing, I cannot approve the patch.
> Sorry. I hate when this happens to me so I do feel your stress and pain,
> Jorgen. :)
>
> Chuck
>
>
