Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
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 br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
On Thu, Feb 14, 2013 at 07:21:27PM -0500, Tom Lane wrote:
Bruce Momjian br...@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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
new file mode 100644
index 2ea3f6e..b8faf9c
*** a/src/bin/initdb/initdb.c
--- b/src/bin/initdb/initdb.c
*** void setup_signals(void);
*** 257,262
--- 257,263
void setup_text_search(void);
void create_data_directory(void);
void create_xlog_symlink(void);
+ void warn_on_mount_point(int error);
void initialize_data_directory(void);
*** setup_signals(void)
*** 3144,3150
void
create_data_directory(void)
{
! switch (pg_check_dir(pg_data))
{
case 0:
/* PGDATA not there, must create it */
--- 3145,3153
void
create_data_directory(void)
{
! int ret;
!
! switch ((ret = pg_check_dir(pg_data)))
{
case 0:
/* PGDATA not there, must create it */
*** create_data_directory(void)
*** 3179,3193
break;
case 2:
/* Present and not empty */
fprintf(stderr,
_(%s: directory \%s\ exists but is not empty\n),
progname, pg_data);
! fprintf(stderr,
! _(If you want to create a new database system, either remove or empty\n
! the directory \%s\ or run %s\n
! with an argument other than \%s\.\n),
! pg_data, progname, pg_data);
exit(1); /* no further message needed */
default:
--- 3182,3201
break;
case 2:
+ case 3:
+ case 4:
/* Present and not empty */
fprintf(stderr,
_(%s: directory \%s\ exists but is not empty\n),
progname, pg_data);
! if (ret != 4)
! warn_on_mount_point(ret);
! else
! fprintf(stderr,
! _(If you want to create a new database system, either remove or empty\n
! the directory \%s\ or run %s\n
! with an argument other than \%s\.\n),
! pg_data, progname, pg_data);
exit(1); /* no further message needed */
default:
*** create_xlog_symlink(void)
*** 3206,3211
--- 3214,3220
if (strcmp(xlog_dir, ) != 0)
{
char *linkloc;
+ int ret;
/* clean up xlog directory name, check it's absolute */
canonicalize_path(xlog_dir);
*** create_xlog_symlink(void)
*** 3216,3222
}
/* check if the specified xlog directory exists/is empty */
! switch (pg_check_dir(xlog_dir))
{
case 0:
/* xlog directory not there, must create it */
--- 3225,3231
}
/* check if the specified xlog directory exists/is empty */
! switch ((ret = pg_check_dir(xlog_dir)))
{
case 0:
/* xlog directory not there, must create it */
*** create_xlog_symlink(void)
*** 3255,3268
break;
case 2:
/* Present and not empty */
fprintf(stderr,
_(%s: directory \%s\ exists but is not empty\n),
progname, xlog_dir);
! fprintf(stderr,
! _(If you want to store the transaction log there, either\n
! remove or empty the directory \%s\.\n),
! xlog_dir);
exit_nicely();
default:
--- 3264,3282
break;
case 2:
+ case 3:
+ case 4:
/* Present and not empty */
fprintf(stderr,
_(%s: directory \%s\ exists but is not empty\n),
progname, xlog_dir);
! if (ret != 4)
! warn_on_mount_point(ret);
! else
! fprintf(stderr,
! _(If you want to store the transaction log there, either\n
! remove or empty the directory \%s\.\n),
! xlog_dir);
exit_nicely();
default:
*** create_xlog_symlink(void)
*** 3291,3296
--- 3305,3325
}
+ void
+ warn_on_mount_point(int error)
+ {
+ if (error == 2)
+ fprintf(stderr,
+ _(It contains a dot-prefixed/invisible file, perhaps due to it being a mount point.\n));
+ else if (error == 3)
+ fprintf(stderr,
+
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
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 s...@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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
new file mode 100644
index 2ea3f6e..03eadcd
*** a/src/bin/initdb/initdb.c
--- b/src/bin/initdb/initdb.c
*** setup_signals(void)
*** 3144,3150
void
create_data_directory(void)
{
! switch (pg_check_dir(pg_data))
{
case 0:
/* PGDATA not there, must create it */
--- 3144,3152
void
create_data_directory(void)
{
! int ret;
!
! switch ((ret = pg_check_dir(pg_data)))
{
case 0:
/* PGDATA not there, must create it */
*** create_data_directory(void)
*** 3179,3193
break;
case 2:
/* Present and not empty */
fprintf(stderr,
_(%s: directory \%s\ exists but is not empty\n),
progname, pg_data);
! fprintf(stderr,
! _(If you want to create a new database system, either remove or empty\n
! the directory \%s\ or run %s\n
! with an argument other than \%s\.\n),
! pg_data, progname, pg_data);
exit(1); /* no further message needed */
default:
--- 3181,3205
break;
case 2:
+ case 3:
+ case 4:
/* Present and not empty */
fprintf(stderr,
_(%s: directory \%s\ exists but is not empty\n),
progname, pg_data);
! if (ret == 2)
! fprintf(stderr,
! _(It contains a dot-prefixed/invisible file.\n));
! else if (ret == 3)
! fprintf(stderr,
! _(It contains a lost+found directory.\n
! Using the top-level directory of a mount point is not recommended.\n));
! if (ret != 3)
! fprintf(stderr,
! _(If you want to create a new database system, either remove or empty\n
! the directory \%s\ or run %s\n
! with an argument other than \%s\.\n),
! pg_data, progname, pg_data);
exit(1); /* no further message needed */
default:
*** create_xlog_symlink(void)
*** 3206,3211
--- 3218,3224
if (strcmp(xlog_dir, ) != 0)
{
char *linkloc;
+ int ret;
/* clean up xlog directory name, check it's absolute */
canonicalize_path(xlog_dir);
*** create_xlog_symlink(void)
*** 3216,3222
}
/* check if the specified xlog directory exists/is empty */
! switch (pg_check_dir(xlog_dir))
{
case 0:
/* xlog directory not there, must create it */
--- 3229,3235
}
/* check if the specified xlog directory exists/is empty */
! switch ((ret = pg_check_dir(xlog_dir)))
{
case 0:
/* xlog directory not there, must create it */
*** create_xlog_symlink(void)
*** 3255,3268
break;
case 2:
/* Present and not empty */
fprintf(stderr,
_(%s: directory \%s\ exists but is not empty\n),
progname, xlog_dir);
! fprintf(stderr,
! _(If you want to store the transaction log there, either\n
! remove or empty the directory \%s\.\n),
! xlog_dir);
exit_nicely();
default:
--- 3268,3291
break;
case 2:
+ case 3:
+ case 4:
/* Present and not empty */
fprintf(stderr,
_(%s: directory \%s\ exists but is not empty\n),
progname, xlog_dir);
! if (ret == 2)
! fprintf(stderr,
! _(It contains a dot-prefixed/invisible file.\n));
! else if (ret == 3)
! fprintf(stderr,
! _(It contains a lost+found directory.\n
! Using the top-level directory of a mount point is not recommended.\n));
! if (ret != 3)
! fprintf(stderr,
! _(If you want to store the transaction log there, either\n
! remove or empty the directory \%s\.\n),
! xlog_dir);
exit_nicely();
default:
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
new file mode 100644
index b6f7744..fb5a1bd
***
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
On Thu, Feb 14, 2013 at 07:21:27PM -0500, Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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. src-port-pgcheckdir.c.patch Description: Binary data 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 s...@chittenden.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
Sean Chittenden s...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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. 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 s...@chittenden.org src-port-pgcheckdir.c.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
On 02/05/2013 07:32 AM, Kevin Grittner wrote: Sean Chittenden s...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
On 2/5/13 7:32 AM, Kevin Grittner wrote: Sean Chittenden s...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
On 5 February 2013 13:50, Craig Ringer cr...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
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 s...@chittenden.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/ports/pgcheckdir.c - Ignore dot directories...
Sean Chittenden s...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
