Re: initdb mkdir_p() doesn't work

Here is what I get:

peter ~$ pg-install/bin/initdb pg-install/var/data
...
creating directory pg-install/var/data ... initdb: failed

No points for details in the error message here either.

If I create pg-install/var first, then it work.

--
Peter Eisentraut peter_e(at)gmx(dot)net

Peter Eisentraut wrote:

>Here is what I get:
>
>peter ~$ pg-install/bin/initdb pg-install/var/data
>...
>creating directory pg-install/var/data ... initdb: failed
>
>No points for details in the error message here either.
>
>If I create pg-install/var first, then it work.
>

I will check it out. I know I spent quite some time making sure this
worked, but I might have missed something obvious. I wonder if it is
platform specific?

cheers

andrew

Andrew Dunstan wrote:

>
>
> Peter Eisentraut wrote:
>
>> Here is what I get:
>>
>> peter ~$ pg-install/bin/initdb pg-install/var/data
>> ...
>> creating directory pg-install/var/data ... initdb: failed
>>
>> No points for details in the error message here either.
>>
>> If I create pg-install/var first, then it work.
>>
>
> I will check it out. I know I spent quite some time making sure this
> worked, but I might have missed something obvious. I wonder if it is
> platform specific?
>
>

I don't remember why the code is the way it is. The failure appears to
be before we ever get to mkdir_p(). I can't see any reason right now why
we can't call mkdir_p() in all cases. The attached patch does that (and
makes the code slightly simpler as a result). I tested it with one
element and 2 element existant and nonexistant paths, and it appeared to
work for all of them.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Peter Eisentraut wrote:
>> creating directory pg-install/var/data ... initdb: failed

> I will check it out. I know I spent quite some time making sure this
> worked, but I might have missed something obvious. I wonder if it is
> platform specific?

AFAICS mkdatadir() shouldn't consider subdir == NULL as a reason to
fail rather than trying mkdir_p. Indeed, if anything the opposite:
when subdir isn't NULL the immediately prior directory level should
exist already.

I concur with Peter's gripe that a perror() or two wouldn't hurt here.

regards, tom lane

Tom Lane wrote:

>AFAICS mkdatadir() shouldn't consider subdir == NULL as a reason to
>fail rather than trying mkdir_p. Indeed, if anything the opposite:
>when subdir isn't NULL the immediately prior directory level should
>exist already.
>

Right. In fact, I can't see any good reason to call mkdir and then
mkdir_p at all. See my patch from this afternoon.

>
>I concur with Peter's gripe that a perror() or two wouldn't hurt here.
>

Sure. Of course, the reason I put this on my web site and asked for
eyeballs was to try to catch some of this sort of stuff before the
program went into the tree :-)

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> AFAICS mkdatadir() shouldn't consider subdir == NULL as a reason to
>> fail rather than trying mkdir_p.

> Right. In fact, I can't see any good reason to call mkdir and then
> mkdir_p at all. See my patch from this afternoon.

I'm unsure about that. I liked the original idea of only trying mkdir_p
when plain mkdir() had failed with ENOENT. I am not convinced your
proposed patch will behave desirably under all error cases. In
particular, mkdir_p seems rather dependent on knowing just which errno
codes will get returned --- which is okay for its heritage as BSD-only
code, but how well will it port? Better to only invoke it when we have
reason to think it can help.

> Sure. Of course, the reason I put this on my web site and asked for
> eyeballs was to try to catch some of this sort of stuff before the
> program went into the tree :-)

We have a whole development cycle to shake these issues out. Don't
panic.

regards, tom lane

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>Tom Lane wrote:
>>
>>
>>>AFAICS mkdatadir() shouldn't consider subdir == NULL as a reason to
>>>fail rather than trying mkdir_p.
>>>
>>>
>
>
>
>>Right. In fact, I can't see any good reason to call mkdir and then
>>mkdir_p at all. See my patch from this afternoon.
>>
>>
>
>I'm unsure about that. I liked the original idea of only trying mkdir_p
>when plain mkdir() had failed with ENOENT. I am not convinced your
>proposed patch will behave desirably under all error cases. In
>particular, mkdir_p seems rather dependent on knowing just which errno
>codes will get returned --- which is okay for its heritage as BSD-only
>code, but how well will it port? Better to only invoke it when we have
>reason to think it can help.
>

OK, then the simple thing to do would be either to change the test on
subdir or to remove it altogether and just check for ENOENT. I'd be
surprised if the code weren't fairly portable, though.

>
>
>
>>Sure. Of course, the reason I put this on my web site and asked for
>>eyeballs was to try to catch some of this sort of stuff before the
>>program went into the tree :-)
>>
>>
>
>We have a whole development cycle to shake these issues out. Don't
>panic.
>
>
>

<alfred.e.newman-mode>What, me panic?</alfred.e.newman-mode>

I started with 2 goals: have initdb work on Unix with "make check", and
have the Win32/Cygwin issues sorted out as far as possible, so when we
get a working w32 postmaster we can actually use it :-). There are 2500
lines of C here - the odd bug isn't surprising.

cheers

andrew

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > Tom Lane wrote:
> >> AFAICS mkdatadir() shouldn't consider subdir == NULL as a reason to
> >> fail rather than trying mkdir_p.
>
> > Right. In fact, I can't see any good reason to call mkdir and then
> > mkdir_p at all. See my patch from this afternoon.
>
> I'm unsure about that. I liked the original idea of only trying mkdir_p
> when plain mkdir() had failed with ENOENT. I am not convinced your
> proposed patch will behave desirably under all error cases. In
> particular, mkdir_p seems rather dependent on knowing just which errno
> codes will get returned --- which is okay for its heritage as BSD-only
> code, but how well will it port? Better to only invoke it when we have
> reason to think it can help.

I am inclined to apply the existing patch and see if we get actual errno
failures from port testing.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Andrew Dunstan wrote:
>
>
> Andrew Dunstan wrote:
>
> >
> >
> > Peter Eisentraut wrote:
> >
> >> Here is what I get:
> >>
> >> peter ~$ pg-install/bin/initdb pg-install/var/data
> >> ...
> >> creating directory pg-install/var/data ... initdb: failed
> >>
> >> No points for details in the error message here either.
> >>
> >> If I create pg-install/var first, then it work.
> >>
> >
> > I will check it out. I know I spent quite some time making sure this
> > worked, but I might have missed something obvious. I wonder if it is
> > platform specific?
> >
> >
>
> I don't remember why the code is the way it is. The failure appears to
> be before we ever get to mkdir_p(). I can't see any reason right now why
> we can't call mkdir_p() in all cases. The attached patch does that (and
> makes the code slightly simpler as a result). I tested it with one
> element and 2 element existant and nonexistant paths, and it appeared to
> work for all of them.
>
> cheers
>
> andrew
>

> ? .deps
> ? initdb
> Index: initdb.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/bin/initdb/initdb.c,v
> retrieving revision 1.11
> diff -c -w -r1.11 initdb.c
> *** initdb.c 17 Nov 2003 20:35:28 -0000 1.11
> --- initdb.c 23 Nov 2003 19:46:56 -0000
> ***************
> *** 797,803 ****
> mkdatadir(char *subdir)
> {
> char *path;
> - int res;
>
> path = xmalloc(strlen(pg_data) + 2 +
> (subdir == NULL ? 0 : strlen(subdir)));
> --- 797,802 ----
> ***************
> *** 807,819 ****
> else
> strcpy(path, pg_data);
>
> ! res = mkdir(path, 0700);
> ! if (res == 0)
> ! return true;
> ! else if (subdir == NULL || errno != ENOENT)
> ! return false;
> ! else
> ! return !mkdir_p(path, 0700);
> }
>
>
> --- 806,812 ----
> else
> strcpy(path, pg_data);
>
> ! return (mkdir_p(path, 0700) == 0);
> }
>
>

>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Patch applied. Thanks.

---------------------------------------------------------------------------

Andrew Dunstan wrote:
>
>
> Andrew Dunstan wrote:
>
> >
> >
> > Peter Eisentraut wrote:
> >
> >> Here is what I get:
> >>
> >> peter ~$ pg-install/bin/initdb pg-install/var/data
> >> ...
> >> creating directory pg-install/var/data ... initdb: failed
> >>
> >> No points for details in the error message here either.
> >>
> >> If I create pg-install/var first, then it work.
> >>
> >
> > I will check it out. I know I spent quite some time making sure this
> > worked, but I might have missed something obvious. I wonder if it is
> > platform specific?
> >
> >
>
> I don't remember why the code is the way it is. The failure appears to
> be before we ever get to mkdir_p(). I can't see any reason right now why
> we can't call mkdir_p() in all cases. The attached patch does that (and
> makes the code slightly simpler as a result). I tested it with one
> element and 2 element existant and nonexistant paths, and it appeared to
> work for all of them.
>
> cheers
>
> andrew
>

> ? .deps
> ? initdb
> Index: initdb.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/bin/initdb/initdb.c,v
> retrieving revision 1.11
> diff -c -w -r1.11 initdb.c
> *** initdb.c 17 Nov 2003 20:35:28 -0000 1.11
> --- initdb.c 23 Nov 2003 19:46:56 -0000
> ***************
> *** 797,803 ****
> mkdatadir(char *subdir)
> {
> char *path;
> - int res;
>
> path = xmalloc(strlen(pg_data) + 2 +
> (subdir == NULL ? 0 : strlen(subdir)));
> --- 797,802 ----
> ***************
> *** 807,819 ****
> else
> strcpy(path, pg_data);
>
> ! res = mkdir(path, 0700);
> ! if (res == 0)
> ! return true;
> ! else if (subdir == NULL || errno != ENOENT)
> ! return false;
> ! else
> ! return !mkdir_p(path, 0700);
> }
>
>
> --- 806,812 ----
> else
> strcpy(path, pg_data);
>
> ! return (mkdir_p(path, 0700) == 0);
> }
>
>

>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073