Re: 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 also the mount point for filesystems that support snapshots (e.g. ZFS or UFS2). The original patch to the FreeBSD ports team excluded ".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 special directories 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 that shouldn't be required. By UNIX convention, files/directories beginning with a dot are hidden anyway, and since PostgreSQL isn'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.

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 special directories 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(at)chittenden(dot)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 any special 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(at)chittenden(dot)org

On 02/05/2013 07:32 AM, Kevin Grittner wrote:
> Sean Chittenden <sean(at)chittenden(dot)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(at)chittenden(dot)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(at)2ndquadrant(dot)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 around that 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, but as 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, 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.

Personally I'm a fan of having PGDATA's parent directory be its own dataset/filesystem as well, but that's because I want PGDATA'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 bad idea, however. *shrug*

Regardless, hopefully some of this is of interest to someone.

-sc

--
Sean Chittenden
sean(at)chittenden(dot)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(at)chittenden(dot)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(at)chittenden(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +