From: Date: July 8 2008 12:30am
Subject: RE: bzr commit into mysql-6.0-backup branch (cbell:2648) Bug#35230
List-Archive: http://lists.mysql.com/commits/49138
Message-Id: <9B8F8558D82F4BE986835A9491889FDD@mysqlcabdesk>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="us-ascii"
Content-Transfer-Encoding: 7bit

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