Re: Btrfs clone WIP patch

Jonathan Rogers <jrogers(at)socialserve(dot)com> writes:
> This patch against PostgreSQL 9.1.8 takes advantage of efficient file
> cloning on Linux Btrfs file systems to make CREATE DATABASE operations
> extremely fast regardless of the size of the database used as a
> template.

It would be easier to review this patch if the bulk of it weren't simple
reindentation of existing code. (Or at least it ought to be that ---
I object to your having moved the buffer palloc inside the loop. A
patch that is trying to optimize a minority case can expect to be
rejected if it makes things worse for everyone else.)

Consider whether you can't phrase the patch to avoid that, perhaps by
use of "continue" instead of an else-block. Alternatively, enclose the
existing code in braces but don't reindent it, ie,

+ if (whatever)
+ ... new code ...
+ else
+ {
... existing code ...
+ }

The next pgindent run will fix the funny indentation, or the committer
can do it if he wishes after reviewing.

> The efficient cloning is accomplished by a Btrfs-specific ioctl() call.

The big-picture question of course is whether we want to carry and
maintain a filesystem-specific hack. I don't have a sense that btrfs
is so widely used as to justify this.

> +#ifdef __linux__
> +# define BTRFS_IOCTL_MAGIC 0x94
> +# define BTRFS_IOC_CLONE _IOW (BTRFS_IOCTL_MAGIC, 9, int)
> + return ioctl (dest_fd, BTRFS_IOC_CLONE, src_fd);
> +#else

This seems to me to be unacceptable on its face. If we can't get these
constants out of a system header file, it's unlikely that the feature is
stable enough to depend on, if indeed it's meant for general-purpose use
at all. We could easily end up invoking unexpected behaviors.

regards, tom lane