Thread: src/ports/pgcheckdir.c - Ignore dot directories...
Hello. This was bounced my way via IRC[1] and I'm kicking an updated version of the patch upstream for review and committing. Currently src/port/pgcheckdir.c will reject non-empty directories, which is an issue during initdb(1) when PGDATA is alsothe mount point for filesystems that support snapshots (e.g. ZFS or UFS2). The original patch to the FreeBSD ports teamexcluded ".snap", but this seems limited. Instead, it seems more correct to simply ignore all directories that begin with a dot character. I'm not aware of any specialdirectories exposed by filesystems that aren't dot directories so this seems like a relatively futureproof solution,too. Granted it's not hard to create a subdirectory, initdb there and move the contents of the files around, it's extra work thatshouldn't be required. By UNIX convention, files/directories beginning with a dot are hidden anyway, and since PostgreSQLisn't using or creating any dot files or directories, this seems like the right trade off in usability. Here's a quick reproduction of the problem along with the patch. Thanks in advance. -sc > # zfs create tank/tmp/pginit-test > # zfs set snapdir=visible tank/tmp/pginit-test > # ll > total 0 > dr-xr-xr-x 4 root wheel 4 Feb 5 08:17 .zfs/ > # su - pgsql > $ initdb -D /tmp/pginit-test > The files belonging to this database system will be owned by user "pgsql". > This user must also own the server process. > > The database cluster will be initialized with locale "C". > The default database encoding has accordingly been set to "SQL_ASCII". > The default text search configuration will be set to "english". > > initdb: directory "/tmp/pginit-test" exists but is not empty > If you want to create a new database system, either remove or empty > the directory "/tmp/pginit-test" or run initdb > with an argument other than "/tmp/pginit-test". [1] http://www.freebsd.org/cgi/query-pr.cgi?pr=ports/174020 -- Sean Chittenden sean@chittenden.org
Attachment
On 02/05/2013 04:36 PM, Sean Chittenden wrote: > Hello. This was bounced my way via IRC[1] and I'm kicking an updated version of the patch upstream for review and committing. > > Instead, it seems more correct to simply ignore all directories that begin with a dot character. I'm not aware of any specialdirectories exposed by filesystems that aren't dot directories so this seems like a relatively futureproof solution,too. lost+found -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Sean Chittenden <sean@chittenden.org> wrote: > Currently src/port/pgcheckdir.c will reject non-empty > directories, which is an issue during initdb(1) when PGDATA is > also the mount point for filesystems that support snapshots (e.g. > ZFS or UFS2). > Granted it's not hard to create a subdirectory, initdb there and > move the contents of the files around, it's extra work that > shouldn't be required. I feel that it is very bad practice to use the mount point as the PGDATA directory. It forcloses a lot of reasonable actions that someone managing the database server might want to take. It's hard to get enthusiastic about a patch to make bad practice more convenient. I would rather add a sentence or two to the initdb documentation recommending that a cluster not be created at a mount point; it should be created in a directory underneath the mount point. -Kevin
>> Hello. This was bounced my way via IRC[1] and I'm kicking an updated version of the patch upstream for review and committing. >> >> Instead, it seems more correct to simply ignore all directories that begin with a dot character. I'm not aware of anyspecial directories exposed by filesystems that aren't dot directories so this seems like a relatively futureproof solution,too. > lost+found It's been a long time since I've seen that directory. Patch updated. -sc -- Sean Chittenden sean@chittenden.org
Attachment
On 02/05/2013 07:32 AM, Kevin Grittner wrote: > Sean Chittenden <sean@chittenden.org> wrote: > >> Currently src/port/pgcheckdir.c will reject non-empty >> directories, which is an issue during initdb(1) when PGDATA is >> also the mount point for filesystems that support snapshots (e.g. >> ZFS or UFS2). >> Granted it's not hard to create a subdirectory, initdb there and >> move the contents of the files around, it's extra work that >> shouldn't be required. > I feel that it is very bad practice to use the mount point as the > PGDATA directory. It forcloses a lot of reasonable actions that > someone managing the database server might want to take. > > It's hard to get enthusiastic about a patch to make bad practice > more convenient. I would rather add a sentence or two to the > initdb documentation recommending that a cluster not be created at > a mount point; it should be created in a directory underneath the > mount point. > I tend to agree. cheers andrew
On 2/5/13 7:32 AM, Kevin Grittner wrote: > Sean Chittenden <sean@chittenden.org> wrote: > >> > Currently src/port/pgcheckdir.c will reject non-empty >> > directories, which is an issue during initdb(1) when PGDATA is >> > also the mount point for filesystems that support snapshots (e.g. >> > ZFS or UFS2). >> > Granted it's not hard to create a subdirectory, initdb there and >> > move the contents of the files around, it's extra work that >> > shouldn't be required. > I feel that it is very bad practice to use the mount point as the > PGDATA directory. It forcloses a lot of reasonable actions that > someone managing the database server might want to take. Yes, a variant of this particular patch gets rejected about once every 18 months.
On 02/05/2013 08:32 PM, Kevin Grittner wrote: > I would rather add a sentence or two to the > initdb documentation recommending that a cluster not be created at > a mount point; it should be created in a directory underneath the > mount point. That makes a great deal of sense, actually. There's no meaningful advantage to creating the cluster at the mountpoint root, and even if you wanted to you could (on Linux) use bind mounts to make it at the root of a mount without being at the root of a filesystem. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 5 February 2013 13:50, Craig Ringer <craig@2ndquadrant.com> wrote: > On 02/05/2013 08:32 PM, Kevin Grittner wrote: > >> I would rather add a sentence or two to the >> initdb documentation recommending that a cluster not be created at >> a mount point; it should be created in a directory underneath the >> mount point. > That makes a great deal of sense, actually. There's no meaningful > advantage to creating the cluster at the mountpoint root, and even if > you wanted to you could (on Linux) use bind mounts to make it at the > root of a mount without being at the root of a filesystem. How about we allow Sean's patch insomuch as it can detect files beginning with dots and throw an error message explaining what the best practice is instead. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
>> Currently src/port/pgcheckdir.c will reject non-empty >> directories, which is an issue during initdb(1) when PGDATA is >> also the mount point for filesystems that support snapshots (e.g. >> ZFS or UFS2). > >> Granted it's not hard to create a subdirectory, initdb there and >> move the contents of the files around, it's extra work that >> shouldn't be required. > > I feel that it is very bad practice to use the mount point as the > PGDATA directory. It forcloses a lot of reasonable actions that > someone managing the database server might want to take. The only reason I have skin in this game is because pg_check_dir(xlog_dir) is called and I routinely have to work aroundthat nuisance/nugget of joy. Again, just a nuisance. It's common to create a ZFS dataset for specific applications (`zfs send ... | ssh zfs receive ...`). In SAN environments,mounting a LUN as PGDATA or pg_xlog isn't uncommon either. I agree it's not ideal for some filesystems, but being overly protective doesn't buy us much either, because in some setups,it's entirely acceptable. If PostgreSQL had the ability to chroot(2) itself, I'd be very opposed to this patch, butas is, it's mostly harmless (the rev that didn't have lost+found, actually). In thinking about it, I like ignoring the hidden directories and failing when lost+found is present because, IMO, filesystemswhere lost+found is going to be present are exactly the filesystems that shouldn't have PGDATA located at thetop of a mount point. Personally I'm a fan of having PGDATA's parent directory be its own dataset/filesystem as well, but that's because I wantPGDATA's parent directory to include PostgreSQL's minor version number (e.g. zpool datasets: tank/pg tank/pg/data tank/pg/data/9.2),but I digress. > It's hard to get enthusiastic about a patch to make bad practice > more convenient. I would rather add a sentence or two to the > initdb documentation recommending that a cluster not be created at > a mount point; it should be created in a directory underneath the > mount point. Giving filesystem advice is a large topic that I'm sure is covered someplace in the handbook. A general warning isn't a badidea, however. *shrug* Regardless, hopefully some of this is of interest to someone. -sc -- Sean Chittenden sean@chittenden.org
Sean Chittenden wrote: > In thinking about it, I like ignoring the hidden directories and failing when lost+found is present > because, IMO, filesystems where lost+found is going to be present are exactly the filesystems that > shouldn't have PGDATA located at the top of a mount point. Huh? What's wrong with ext3 or ext4? Yours, Laurenz Albe
Sean Chittenden <sean@chittenden.org> writes:
> I agree it's not ideal for some filesystems, but being overly protective doesn't buy us much either, because in some
setups,it's entirely acceptable.
No, it isn't. As several people have told you already, the idea of
letting a mount point be used directly as a data directory has been
suggested repeatedly, and rejected repeatedly, and this time is not
going to be any different. (Although I agree with Kevin that it's
about time we documented why not to do this.)
There are a couple of reasons why it's not good practice:
* mount-point directories really ought to be owned by root, or at least
by some user with more privilege than a DB server ought to have
* without a sub-directory, there's no simple cross-check to enforce that
the mount has actually happened. It's happened before that people have
had a server start up against a slow-to-mount NFS directory, and then
get completely confused when the mount did happen and the visible
database files got replaced. (The really nasty variants of this require
a startup script that will try to initdb automatically if it doesn't see
a database there.)
That's just what I can remember off the top of my head with insufficient
caffeine. If you check the archives for previous discussions you might
find some other good points.
regards, tom lane
On Tue, Feb 5, 2013 at 08:49:17AM -0500, Peter Eisentraut wrote: > On 2/5/13 7:32 AM, Kevin Grittner wrote: > > Sean Chittenden <sean@chittenden.org> wrote: > > > >> > Currently src/port/pgcheckdir.c will reject non-empty > >> > directories, which is an issue during initdb(1) when PGDATA is > >> > also the mount point for filesystems that support snapshots (e.g. > >> > ZFS or UFS2). > >> > Granted it's not hard to create a subdirectory, initdb there and > >> > move the contents of the files around, it's extra work that > >> > shouldn't be required. > > I feel that it is very bad practice to use the mount point as the > > PGDATA directory. It forcloses a lot of reasonable actions that > > someone managing the database server might want to take. > > Yes, a variant of this particular patch gets rejected about once every > 18 months. Agreed. The attached patch modifies pg_check_dir() to report about invisible and lost+found directory entries, and give more helpful messages to the user. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Attachment
Bruce Momjian <bruce@momjian.us> writes:
> Agreed. The attached patch modifies pg_check_dir() to report about
> invisible and lost+found directory entries, and give more helpful
> messages to the user.
I'm not terribly thrilled with special-casing 'lost+found' like that,
since it's an extremely filesystem-dependent thing that even today
probably only applies to a minority of our installed platforms.
The special case for dotfiles might be useful, not because of any
connection to mount points but just because someone might forget
that such could be lurking in a directory that "looks empty".
regards, tom lane
On Thu, Feb 14, 2013 at 07:21:27PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Agreed. The attached patch modifies pg_check_dir() to report about > > invisible and lost+found directory entries, and give more helpful > > messages to the user. > > I'm not terribly thrilled with special-casing 'lost+found' like that, > since it's an extremely filesystem-dependent thing that even today > probably only applies to a minority of our installed platforms. > > The special case for dotfiles might be useful, not because of any > connection to mount points but just because someone might forget > that such could be lurking in a directory that "looks empty". Yeah, I agree on both points. I am not sure the patch is worth it just the dot output. Want a crazy idea? '.' and '..' have different major device numbers on the top directory of a mount point. We could test for that and prevent/warn about creating data directories on top-level directories of mount points. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Feb 14, 2013 at 07:21:27PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Agreed. The attached patch modifies pg_check_dir() to report about > > invisible and lost+found directory entries, and give more helpful > > messages to the user. > > I'm not terribly thrilled with special-casing 'lost+found' like that, > since it's an extremely filesystem-dependent thing that even today > probably only applies to a minority of our installed platforms. > > The special case for dotfiles might be useful, not because of any > connection to mount points but just because someone might forget > that such could be lurking in a directory that "looks empty". I was ready to give up on this patch, but then I thought, what percentage does lost+found and dot-file-only directories cover for mount points? What other cases are there? This updated version of the patch reports about dot files if they are the _only_ files in the directory, and it suggests a top-level mount point might be the cause. Does this help? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Attachment
On Fri, Feb 15, 2013 at 12:12:03PM -0500, Bruce Momjian wrote: > On Thu, Feb 14, 2013 at 07:21:27PM -0500, Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > Agreed. The attached patch modifies pg_check_dir() to report about > > > invisible and lost+found directory entries, and give more helpful > > > messages to the user. > > > > I'm not terribly thrilled with special-casing 'lost+found' like that, > > since it's an extremely filesystem-dependent thing that even today > > probably only applies to a minority of our installed platforms. > > > > The special case for dotfiles might be useful, not because of any > > connection to mount points but just because someone might forget > > that such could be lurking in a directory that "looks empty". > > I was ready to give up on this patch, but then I thought, what > percentage does lost+found and dot-file-only directories cover for mount > points? What other cases are there? > > This updated version of the patch reports about dot files if they are > the _only_ files in the directory, and it suggests a top-level mount > point might be the cause. > > Does this help? Applied. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +