maria-developers team mailing list archive

[Laurynas Biveinis <laurynas.biveinis@xxxxxxxxx>]

Hi -

We ported your patch to Percona Server, meanwhile we collected the
following comments which perhaps are useful for you. These are
addressed in our XtraDB port, so you will pick them up, but since you
patched InnoDB as well, you might consider addressing them there as
well. I tried to order them roughly in the order of importance.

The comments below refer to rev 3720 of 5.5 tree (and one comment to
MariaDB 10 trunk).

- Where are the testcases? sys_vars test doesn't count :)

- fil_extend_space_to_desired_
size() fails to consider that it might
be extending a single node on a multi-node tablespace. It will
calculate the desired size as if the last node was the only one,
overextending the tablespace by the size of all the previous nodes.
Fix is replacing
        offset_high = size_after_extend * page_size / (4ULL*1024*1024*1024);
        offset_low = size_after_extend * page_size % (4ULL*1024*1024*1024);
with
        offset_high = (size_after_extend - file_start_page_no)
            * page_size / (4ULL * 1024 * 1024 * 1024);
        offset_low = (size_after_extend - file_start_page_no)
            * page_size % (4ULL * 1024 * 1024 * 1024);

- In the same function in InnoDB 5.6 / MariaDB 10.0 it is necessary to set
node->being_extended = FALSE; before jumping to complete_io.

- In the same function, complete_io: label may skip over the
fil_node_complete_io() call, which is required for os_aio()
tablespace-extending call only.

- os_file_set_atomic_writes() should return not an int, but bool or
ibool instead, also its return value currently is raw ioctl() result,
which is used as a bool at callers anyway and reversed from the InnoDB
convention of true/non-zero meaning success. While at that, it's also
nice to add __attribute__((warn_unused_result)) to it.

- If ioctl() failed, then it's customary for the failed I/O call
handling to call one of the os_file_handle_error... functions, here
os_file_handle_error_no_exit() looks like a good fit.

- If atomic writes are unsupported, then I'm not sure why set errno /
LastError. No syscall has failed, the atomic writes unsupported-ness
is communicated by the os_file_set_atomic_writes() return value.

- os_file_set_atomic_writes() declaration should not depend on
__linux__ etc, thus we pushed the #ifdef to function body.

- InnoDB uses UNIV_LINUX instead of __linux__, the build sys checks
for the presence of sys/ioctl.h and defines corresponding define, thus
we replaced

#ifdef __linux__
#include <sys/ioctl.h>
#ifndef DFS_IOCTL_ATOMIC_WRITE_SET
#define DFS_IOCTL_ATOMIC_WRITE_SET _IOW(0x95, 2, uint)
#endif

with

#if defined(UNIV_LINUX) && defined(HAVE_SYS_IOCTL_H)
# include <sys/ioctl.h>
# ifndef DFS_IOCTL_ATOMIC_WRITE_SET
#  define DFS_IOCTL_ATOMIC_WRITE_SET _IOW(0x95, 2, uint)
# endif
#endif

at the top of the file, and the #ifdef inside the function is #ifdef
DFS_IOCTL...

- Is this bit inside __WIN__ in os_file_create_func() actually needed?

    if (srv_use_atomic_writes && type == OS_DATA_FILE &&
        os_file_set_atomic_writes(file, name)) {
             CloseHandle(file);
            *success = FALSE;
            file = INVALID_HANDLE_VALUE;
    }

- os_file_set_size() could call os_file_handle_error_no_exit() for
failed posix_fallocate() instead of printing the errno itself.

- Comments don't follow InnoDB style, missing header function
comments, too long lines, spurious whitespace, etc.

Our code is at
https://bazaar.launchpad.net/~percona-core/percona-server/5.5/revision/518
https://bazaar.launchpad.net/~percona-core/percona-server/5.6/revision/353

We haven't addressed the testcase issue yet neither.

Regards,
--
Laurynas