← Back to team overview

drizzle-discuss team mailing list archive

Re: file system storage engine


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.

- FilesystemCursor::position()
  - avoid using things in the drizzled::internal namespace. Instead you
    can just cast ref to an off_t and store it in there.

- FilesystemCursor::openUpdateFile()
  - on startup you should remove the .UPDATE file if it exists

- 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.

- doInsertRecord()
  - you should check the return code of write(2) lots of interesting
    things can go wrong.

- ::validateCreateTableOption()
  - 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)

- FilesystemEngine::FilesystemEngine()
  - the mutex is destroyed in the destructor, it should be created in
    the constructor.

- ::get_share()
  - use an ENUM for share->separator mode instead of number.

I hope to have a closer look at the update/deletion code shortly.

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.


Stewart Smith

Follow ups