Rafal,
> I request some changes to the patch before it can be pushed.
> These are related to unnecessary memory allocations and
> string copying in the code.
NP. Will be changed.
> Another issue I raise is the fact that you refer to
> thd->lex->backup_dir in the code. This creates another
> unwanted dependency between backup code and the rest of the
> server, as I explain below. I propose a solution for that,
> which is consequently using the "raw" name of the backup
> file, as specified by the user, and adding the backupdir to
> it only inside the backup::Stream class, where the
> corresponding file must be opened and a full path to it must be known.
>
> As we discussed before, an ultimate implementation of
> --backupdir and other backup related options would require
> designing a general mechanism for passing options from server
> to the online backup module. However, we need --backupdir
> option now and we don't have any such mechanism at hand. So I
> accept pushing this code as a temporary solution (given that
> other concerns are resolved), keeping in mind that we would
> have to refactor it when general backup options handling
> mechanism is developed.
Right. I will add comments to the code, the patch, and the worklog. :)
> See more detailed comments below.
...
> One way of thinking about the backupdir setting is that it
> changes semantics of the backup locations specified by users
> in BACKUP/RESTORE commands. Thus 'test.bak' doesn't mean
> "file test.bak in the current working directory" but it
> rather means "file test.bak in the default backup directory".
> Consequently, when one creates Output_stream object passing
> "test.bak" as the name of the file, it should be understood
> as "open a stream for writing to file test.bak in the default
> backup directory". Thus, the backupdir path should be added
> to the name inside Output_stream constructor which must
> translate name 'test.bak' from the backup semantics (i.e.,
> "file test.bak in the default backup directory") to the
> system wide semantics used by low level functions (i.e., "file
> /path/do/default/backup/dir/test.bak")
>
> Thus I suggest to do the path transformation inside
> constructor of the backup::Stream class (so that it will be
> shared by Input/Output_stream.
That is acceptable to me -- provided I can get it to work correctly. ;)
...
> In a more modular design, these functions could be provided
> by the online backup module. The server will only ensure that
> they are called when the variable is accessed.
>
> That is, this would be a design where server still needs to
> be aware that there is a variable called backupdir and that
> it belongs to the online backup module.
>
> Even more modular design would be such that modules, when
> activated, inform server about new variables/options they
> support and server will extend the set of available ones
> dynamically. But that is perhaps too ambitious...
While I agree in principle, I think we are stuck with this one. It is simply
how dynamic variables work (currently) :P.
Chuck
