Re: Btrfs clone WIP patch

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Jonathan Rogers <jrogers(at)socialserve(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Btrfs clone WIP patch
Date: 2013-02-13 22:13:24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Jonathan Rogers 2013-02-13 22:45:46 Re: Btrfs clone WIP patch
Previous Message Alvaro Herrera 2013-02-13 21:29:08 Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system