drizzle-discuss team mailing list archive
Mailing list archive
Re: file system storage engine
Thanks for the review :-)
Replies are inline.
On 24/06/2010 3:38 PM, Stewart Smith wrote:
thanks for reminding. so I need as well to add these checks for write(2)
On Fri, 18 Jun 2010 13:45:54 +0800, ZQ<ziminq@xxxxxxxxx> wrote:
could you please do some review over my branch? It would be helpful.
Sorry this has taken a little while to get back to you, I've been in
training all week. Hope you were still able to get some things done.
Cool! I've spotted a few things for you to take a look at:
- FilesystemCursor::close should check the return value of close(),
especially for EINTR.
- on non-EINTR errors, you can probably just return them and kind of
hope that the upper layer reports these correctly. Since with buffered
IO close(2) can also return all the errors that write() can, things
like EIO may occur here.
it seems it's a convention to check the return value against EINTR.
- avoid using things in the drizzled::internal namespace. Instead you
can just cast ref to an off_t and store it in there.
- on startup you should remove the .UPDATE file if it exists
Ah, I forgot this point. :(
storing '0' for NULL is temporary step, because I'm not sure which one
defaulting to NULL, while this field is actually an INT, this might be a
- getAllFields() is a bit misleadingly named. Perhaps recordToString() ?
- is storing '0' for NULL a good idea? Perhaps another engine
option. defaulting to 'NULL' is probably better too.
- you should check the return code of write(2) lots of interesting
things can go wrong.
same as close(2), will check it.
I'm checking the file permission in validateCreateTableOption() because
I want to do a quick fail over,
- instead of validating access to the file in
validateCreateTableOption (which can only return 'success' or
'failure'), check this in ::doCreateTable which can return a real
error message (e.g. ENOENT, EPERM, EIO etc)
if the corresponding file doesn't exist, the table can't be created.
doing this check in ::doCreateTable is ok, as long as we permit user
can create the table even the file doesn't exist.
- the mutex is destroyed in the destructor, it should be created in
I did it in plugin's init function 'filesystem_init_func'.
But creating it in constructor and destroying it in destructor seems
- use an ENUM for share->separator mode instead of number.
I hope to have a closer look at the update/deletion code shortly.
This comes from my previous refactor of CSV code. I like that idea.
hopefully this helps. things are looking pretty good. One idea may be to
go through the 'load data infile' tests, copy them and rewrite parts to
use the filesystem engine instead.
Will look into this 'load data infile'.