Re: [HACKERS] initdb

I now have a C implementation of initdb, which successfully runs with
"make check" on my several linux machines, and compiles on Windows/MinGW
too (can't run make check on Windows because we haven't got a native
postgres yet - I'm going to create a small dummy Windows postgres that
will let me check if this program works there).

There's a little work still to go (see below), but I'd appreciate some
eyeballs on it to see if I have made any major booboos, or could have
done things better. What's the best way to proceed? (All told it's about
2500 lines of C.)

cheers

andrew

From the heading comment:

/ *
* initdb
*
* This is a C implementation of the previous shell script for setting up a
* PostgreSQL cluster location, and should be highly compatible with it.
*
* TODO:
* - signal handling
* - more error checking, partiularly on the file i/o
* - check if we need workaround for timing error on win32 rmdir()?
* - clean up find_postgres code and return values
* - free up used memory? (probably not worth it - if we can't load this
* much data into memory how will we ever run postgres anyway?)
*/

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I now have a C implementation of initdb, ...
> There's a little work still to go (see below), but I'd appreciate some
> eyeballs on it to see if I have made any major booboos, or could have
> done things better. What's the best way to proceed?

Put it on a web page and post the link, or if you don't have a website,
send it to pgsql-patches (clearly marked as "not ready to apply").

regards, tom lane

Excellent idea.

Here's the URL: http://www.dunslane.net/~andrew/Initdb_In_C.html

cheers

andrew

----- Original Message -----
From: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>

> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > I now have a C implementation of initdb, ...
> > There's a little work still to go (see below), but I'd appreciate some
> > eyeballs on it to see if I have made any major booboos, or could have
> > done things better. What's the best way to proceed?
>
> Put it on a web page and post the link, or if you don't have a website,
> send it to pgsql-patches (clearly marked as "not ready to apply").
>

I have added this URL to the Win32 page, and removed the "initdb C" TODO
item. Great job. Do you wantt this in Win32 CVS or should we just wait
for 7.5 to start?

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

Andrew Dunstan wrote:
>
> Excellent idea.
>
> Here's the URL: http://www.dunslane.net/~andrew/Initdb_In_C.html
>
> cheers
>
> andrew
>
> ----- Original Message -----
> From: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>
> > Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > > I now have a C implementation of initdb, ...
> > > There's a little work still to go (see below), but I'd appreciate some
> > > eyeballs on it to see if I have made any major booboos, or could have
> > > done things better. What's the best way to proceed?
> >
> > Put it on a web page and post the link, or if you don't have a website,
> > send it to pgsql-patches (clearly marked as "not ready to apply").
> >
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>

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

There will be a new version out there soon. When I am happy with it I will
let you know - right now I am dealing with 2 issues - setlocale(LC_ALL,"")
doesn't read from the environment on Windows, and the program is possibly
not picking up the buffers/connections properly.

Please don't put the version currently out there in CVS - the files won't
even be the same - initdb-scripts.h will disappear and there will be a new
file called "system_views.sql".

There is no urgency about this - until we have a working postgres executable
on Windows initdb is useless anyway. My aim has been to have initdb ready
when postgres is ready. Of course, I test against Unix all the time to make
sure nothing gets broken.

cheers

andrew

----- Original Message -----
From: "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: "Andrew Dunstan" <andrew(at)dunslane(dot)net>
Cc: "Postgresql Hackers" <pgsql-hackers(at)postgresql(dot)org>;
"pgsql-hackers-win32" <pgsql-hackers-win32(at)postgresql(dot)org>
Sent: Thursday, October 09, 2003 9:39 PM
Subject: Re: [pgsql-hackers-win32] [HACKERS] initdb

>
> I have added this URL to the Win32 page, and removed the "initdb C" TODO
> item. Great job. Do you wantt this in Win32 CVS or should we just wait
> for 7.5 to start?
>
> --------------------------------------------------------------------------
-
>
> Andrew Dunstan wrote:
> >
> > Excellent idea.
> >
> > Here's the URL: http://www.dunslane.net/~andrew/Initdb_In_C.html
> >
> > cheers
> >
> > andrew
> >
> > ----- Original Message -----
> > From: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> >
> > > Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > > > I now have a C implementation of initdb, ...
> > > > There's a little work still to go (see below), but I'd appreciate
some
> > > > eyeballs on it to see if I have made any major booboos, or could
have
> > > > done things better. What's the best way to proceed?
> > >
> > > Put it on a web page and post the link, or if you don't have a
website,
> > > send it to pgsql-patches (clearly marked as "not ready to apply").
> > >
> >
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 6: Have you searched our list archives?
> >
> > http://archives.postgresql.org
> >
>
> --
> 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
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings

Andrew Dunstan wrote:
> There will be a new version out there soon. When I am happy with it I will
> let you know - right now I am dealing with 2 issues - setlocale(LC_ALL,"")
> doesn't read from the environment on Windows, and the program is possibly
> not picking up the buffers/connections properly.
>
> Please don't put the version currently out there in CVS - the files won't
> even be the same - initdb-scripts.h will disappear and there will be a new
> file called "system_views.sql".
>
> There is no urgency about this - until we have a working postgres executable
> on Windows initdb is useless anyway. My aim has been to have initdb ready
> when postgres is ready. Of course, I test against Unix all the time to make
> sure nothing gets broken.

Fine. We have the URL on the win32 web site, and that's all we need.

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

There's a new version available at
http://www.dunslane.net/~andrew/Initdb_In_C.html - I think it takes care of
all of Peter's comments. I removed initdb-scripts.h - the smaller scripts
were moved into initdb.c with SQL comments turned into C comments, and the
large script to set up system views becomes its own file, which is now
passed to postgres with single line mode turned off.

I have just realised that I seem to have skipped the bki file version check.
I will fix that in the next version.

cheers

andrew

----- Original Message -----
From: "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: "Andrew Dunstan" <andrew(at)dunslane(dot)net>
Cc: "Postgresql Hackers" <pgsql-hackers(at)postgresql(dot)org>;
"pgsql-hackers-win32" <pgsql-hackers-win32(at)postgresql(dot)org>
Sent: Thursday, October 09, 2003 10:07 PM
Subject: Re: [pgsql-hackers-win32] [HACKERS] initdb

> Andrew Dunstan wrote:
> > There will be a new version out there soon. When I am happy with it I
will
> > let you know - right now I am dealing with 2 issues -
setlocale(LC_ALL,"")
> > doesn't read from the environment on Windows, and the program is
possibly
> > not picking up the buffers/connections properly.
> >
> > Please don't put the version currently out there in CVS - the files
won't
> > even be the same - initdb-scripts.h will disappear and there will be a
new
> > file called "system_views.sql".
> >
> > There is no urgency about this - until we have a working postgres
executable
> > on Windows initdb is useless anyway. My aim has been to have initdb
ready
> > when postgres is ready. Of course, I test against Unix all the time to
make
> > sure nothing gets broken.
>
> Fine. We have the URL on the win32 web site, and that's all we need.
>
> --
> 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
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings

Bruce,

I think this is now ready to import into the WIN32 branch. I have ironed out
the cygwin dependencies, and posted a note about what we'll need to do
inside postgres to handle the environment values for setlocale() on Windows.

If you want to rip out the unlink stuff (see previous email thread) we'll
need a version of dirmod.c that is compiled with -DFRONTEND - I guess libpq
is the obvious place for that.

Apart from initdb.c we'll need the file system_views.sql, which can be taken
without change (unless someone want to add comments to it), and Makefile
modifications (see my Makefile on the web for an example, but it probably
doesn't comply with our standard way of doing things).

Anything broken can then be fixed with patches - I have about 7 versions on
5 machines now and my head is starting to swim -).

pg_id and pg_encoding now become redundant in their entirety, as does
initdb.sh.

cheers

andrew

----- Original Message -----
From: "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: "Andrew Dunstan" <andrew(at)dunslane(dot)net>
Cc: "Postgresql Hackers" <pgsql-hackers(at)postgresql(dot)org>;
"pgsql-hackers-win32" <pgsql-hackers-win32(at)postgresql(dot)org>
Sent: Thursday, October 09, 2003 10:07 PM
Subject: Re: [pgsql-hackers-win32] [HACKERS] initdb

> Andrew Dunstan wrote:
> > There will be a new version out there soon. When I am happy with it I
will
> > let you know - right now I am dealing with 2 issues -
setlocale(LC_ALL,"")
> > doesn't read from the environment on Windows, and the program is
possibly
> > not picking up the buffers/connections properly.
> >
> > Please don't put the version currently out there in CVS - the files
won't
> > even be the same - initdb-scripts.h will disappear and there will be a
new
> > file called "system_views.sql".
> >
> > There is no urgency about this - until we have a working postgres
executable
> > on Windows initdb is useless anyway. My aim has been to have initdb
ready
> > when postgres is ready. Of course, I test against Unix all the time to
make
> > sure nothing gets broken.
>
> Fine. We have the URL on the win32 web site, and that's all we need.
>
> --
> 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
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

Andrew Dunstan wrote:
>
> Bruce,
>
> I think this is now ready to import into the WIN32 branch. I have ironed out
> the cygwin dependencies, and posted a note about what we'll need to do
> inside postgres to handle the environment values for setlocale() on Windows.
>
> If you want to rip out the unlink stuff (see previous email thread) we'll
> need a version of dirmod.c that is compiled with -DFRONTEND - I guess libpq
> is the obvious place for that.
>
> Apart from initdb.c we'll need the file system_views.sql, which can be taken
> without change (unless someone want to add comments to it), and Makefile
> modifications (see my Makefile on the web for an example, but it probably
> doesn't comply with our standard way of doing things).
>
> Anything broken can then be fixed with patches - I have about 7 versions on
> 5 machines now and my head is starting to swim -).
>
> pg_id and pg_encoding now become redundant in their entirety, as does
> initdb.sh.

Here is a slightly modified version of Andrew's great work in making a C
version of initdb. Other than minor cleanups, the only big change was
to remove rmdir handling because we using rm -r and rmdir /s in
commands/dbcommands.c, so we might as use the same thing for initdb.c
rather than having code that traverses the directory tree doing 'rm'.

The other change was to remove the unlink code and instead use
port/dirmod.c's version.

It passes all the regression tests. I have also included a diff against
Andrew's version so you can see my changes. It seems Andrew had a very
current version of initdb. The only update he missed was the change to
test the number of connections before shared buffers --- I made that
change myself.

I would like to apply this in the next few days to HEAD before initdb.sh
drifts. We are no longer using the WIN32_DEV branch because all our
work is now in 7.5 head.

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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> It passes all the regression tests. I have also included a diff against
> Andrew's version so you can see my changes. It seems Andrew had a very
> current version of initdb. The only update he missed was the change to
> test the number of connections before shared buffers --- I made that
> change myself.

I had some concern about whether Andrew's rewrite had tracked all the
recent changes to the initdb shell-script sources. Are you both
confident that it is up to date?

regards, tom lane

Tom Lane wrote:

>Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>
>
>>It passes all the regression tests. I have also included a diff against
>>Andrew's version so you can see my changes. It seems Andrew had a very
>>current version of initdb. The only update he missed was the change to
>>test the number of connections before shared buffers --- I made that
>>change myself.
>>
>>
>
>I had some concern about whether Andrew's rewrite had tracked all the
>recent changes to the initdb shell-script sources. Are you both
>confident that it is up to date?
>
>
>

Yes. Bruce has picked up the one change I didn't track (revision 1.204).

cheers

andrew

Bruce Momjian wrote:

>It passes all the regression tests. I have also included a diff against
>Andrew's version so you can see my changes. It seems Andrew had a very
>current version of initdb. The only update he missed was the change to
>test the number of connections before shared buffers --- I made that
>change myself.
>
>I would like to apply this in the next few days to HEAD before initdb.sh
>drifts. We are no longer using the WIN32_DEV branch because all our
>work is now in 7.5 head.
>
>
>

I will double check this over the weekend.

cheers

andrew

Bruce Momjian wrote:

>Here is a slightly modified version of Andrew's great work in making a C
>version of initdb. Other than minor cleanups, the only big change was
>to remove rmdir handling because we using rm -r and rmdir /s in
>commands/dbcommands.c, so we might as use the same thing for initdb.c
>rather than having code that traverses the directory tree doing 'rm'.
>
>The other change was to remove the unlink code and instead use
>port/dirmod.c's version.
>
>It passes all the regression tests. I have also included a diff against
>Andrew's version so you can see my changes. It seems Andrew had a very
>current version of initdb. The only update he missed was the change to
>test the number of connections before shared buffers --- I made that
>change myself.
>
>I would like to apply this in the next few days to HEAD before initdb.sh
>drifts. We are no longer using the WIN32_DEV branch because all our
>work is now in 7.5 head.
>
>

My comments:

I have no problem with shelling out to rmdir - although my goal was to
avoid shelling out to anything other than postgres itself. I think
recreating the datadir if we didn't create it initially should be OK in
that case, and it makes the code simpler. Not handling dot files in the
Unix case should also be fine, as we know the directory was empty to
start with and we don't create any.

Regarding the #endif comments you removed - Peter had asked me to put
them in. See
http://archives.postgresql.org/pgsql-hackers/2003-10/msg00352.php

You put a comment on canonicalise_path() asking if it is needed. The
short answer is very much "yes". The Windows command processor will
accept suitably quoted paths with forward slashes, but barfs badly with
mixed forward and back slashes. (See recent discussion on
hackers-win32).Removing the trailing slash on a path means we never get
ugly double slashes. Feel free to put that info in as a comment if you
think it is needed.

The changes you made for the final message are not going to work for
Windows - if pgpath or pg_data contain spaces we need quotes (even on
Unix, although there most people know better than to put spaces in
names). Also see above about mixed slashes. Also, putting a \ before
pg_data will be nasty if it starts with a drive or network specifier -
or even without (you might turn a directory specifier into a network
drive specifier). It's just a message, though, and we shouldn't hold up
for that - we can patch it to get it right.

(Getting the path and slash thing working portably was by far the
biggest headache in this project - the rest was just grunt work for the
most part, although I learned a few interesting things.)

Otherwise I'm fine with this.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>
> >Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> >
> >
> >>It passes all the regression tests. I have also included a diff against
> >>Andrew's version so you can see my changes. It seems Andrew had a very
> >>current version of initdb. The only update he missed was the change to
> >>test the number of connections before shared buffers --- I made that
> >>change myself.
> >>
> >>
> >
> >I had some concern about whether Andrew's rewrite had tracked all the
> >recent changes to the initdb shell-script sources. Are you both
> >confident that it is up to date?
> >
> >
> >
>
> Yes. Bruce has picked up the one change I didn't track (revision 1.204).

Yes, I was concerned too that everything was in there. I checked the
initdb.sh logs and found that the only thing not added was the checking
of the max number of connections before checking the max number of
buffers, which I added. The other stuff was in there. I also checked
pg_id's recent changes and those were in there too.

Andrew, I assume this was a new implementation of initdb, and not taken
from an older initdb C implementation made by a company.

This isn't really a patch but a C replacement of a critical shell
script so there is reason to double-check things.

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

Bruce Momjian wrote:

>Yes, I was concerned too that everything was in there. I checked the
>initdb.sh logs and found that the only thing not added was the checking
>of the max number of connections before checking the max number of
>buffers, which I added. The other stuff was in there. I also checked
>pg_id's recent changes and those were in there too.
>
>Andrew, I assume this was a new implementation of initdb, and not taken
>from an older initdb C implementation made by a company.
>
>This isn't really a patch but a C replacement of a critical shell
>script so there is reason to double-check things.
>
>

Yes, I worked from initdb.sh, not from any other source. It's "all my
own work" :-) I think I started with 1.201 and later upgraded to 1.203.

I agree it needs careful checking - the more eyeballs the better.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
>
> >Here is a slightly modified version of Andrew's great work in making a C
> >version of initdb. Other than minor cleanups, the only big change was
> >to remove rmdir handling because we using rm -r and rmdir /s in
> >commands/dbcommands.c, so we might as use the same thing for initdb.c
> >rather than having code that traverses the directory tree doing 'rm'.
> >
> >The other change was to remove the unlink code and instead use
> >port/dirmod.c's version.
> >
> >It passes all the regression tests. I have also included a diff against
> >Andrew's version so you can see my changes. It seems Andrew had a very
> >current version of initdb. The only update he missed was the change to
> >test the number of connections before shared buffers --- I made that
> >change myself.
> >
> >I would like to apply this in the next few days to HEAD before initdb.sh
> >drifts. We are no longer using the WIN32_DEV branch because all our
> >work is now in 7.5 head.
> >
> >
>
> My comments:
>
> I have no problem with shelling out to rmdir - although my goal was to
> avoid shelling out to anything other than postgres itself. I think
> recreating the datadir if we didn't create it initially should be OK in
> that case, and it makes the code simpler. Not handling dot files in the
> Unix case should also be fine, as we know the directory was empty to
> start with and we don't create any.

Good. I can do rmdir() in C in port/dirmod.c if we need it. Right now
we are doing system(rm/rmdir) in dbcommands.c so we should consistent.
Let's stay with system(rm/rmdir) and if it doesn't work as we expect, we
can add your rmdir() code and put it in port/dirmod.c.

> Regarding the #endif comments you removed - Peter had asked me to put
> them in. See
> http://archives.postgresql.org/pgsql-hackers/2003-10/msg00352.php

Peter, I thought we add:

#endif /* port... */

only when the define covers many lines of code. This case:

#ifdef WIN32
some code
#endif

seems clearer than:

#ifdef WIN32
some code
#endif /* WIN32 */

Of course, if the code block is very large, a closing comment can
sometimes help. Personally, I don't like the comments anytime, but if
people like them on long blocks, I can understand that.

> You put a comment on canonicalise_path() asking if it is needed. The
> short answer is very much "yes". The Windows command processor will
> accept suitably quoted paths with forward slashes, but barfs badly with
> mixed forward and back slashes. (See recent discussion on
> hackers-win32).Removing the trailing slash on a path means we never get
> ugly double slashes. Feel free to put that info in as a comment if you
> think it is needed.

Done.

> The changes you made for the final message are not going to work for
> Windows - if pgpath or pg_data contain spaces we need quotes (even on
> Unix, although there most people know better than to put spaces in
> names). Also see above about mixed slashes. Also, putting a \ before
> pg_data will be nasty if it starts with a drive or network specifier -
> or even without (you might turn a directory specifier into a network
> drive specifier). It's just a message, though, and we shouldn't hold up
> for that - we can patch it to get it right.

I am confused. The only change I made was to have the quotes appear
only on WIN32.

#ifndef WIN32
#define QUOTE_PATH ""
#else
#define QUOTE_PATH "\""
#endif

...

printf("\n"
"Success. You can now start the database server using:\n\n"
" %s/postmaster -D %s%s%s\n"
"or\n"
" %s/pg_ctl -D %s%s%s -l logfile start\n\n",
pgpath, QUOTE_PATH, pg_data, QUOTE_PATH,
pgpath, QUOTE_PATH, pg_data, QUOTE_PATH);

(I also merged multiple \n into a single line.) My logic was that
spaces in directory names are much more common on WIN32, so we should
display the quotes only on WIN32. Perhaps you read "\"" as "\\"?

> (Getting the path and slash thing working portably was by far the
> biggest headache in this project - the rest was just grunt work for the
> most part, although I learned a few interesting things.)
>
> Otherwise I'm fine with this.

Thanks for reviewing my work. I am surprised how small the new initdb.c
is. It is 50k vs 38k for initdb.sh. Of course, system_views.sql is
another 10k, but still, it is smaller than I thought, and quite clear.

Thanks.

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

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
>
> >Yes, I was concerned too that everything was in there. I checked the
> >initdb.sh logs and found that the only thing not added was the checking
> >of the max number of connections before checking the max number of
> >buffers, which I added. The other stuff was in there. I also checked
> >pg_id's recent changes and those were in there too.
> >
> >Andrew, I assume this was a new implementation of initdb, and not taken
> >from an older initdb C implementation made by a company.
> >
> >This isn't really a patch but a C replacement of a critical shell
> >script so there is reason to double-check things.
> >
> >
>
>
> Yes, I worked from initdb.sh, not from any other source. It's "all my
> own work" :-) I think I started with 1.201 and later upgraded to 1.203.
>
> I agree it needs careful checking - the more eyeballs the better.

The great part is that it look so much like our code, unlike the
commerical port code I have seen for initdb in the past. This certainly
moves us forward on Win32.

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

Andrew Dunstan writes:

> recreating the datadir if we didn't create it initially should be OK in
> that case, and it makes the code simpler.

That should be avoided, because you'll have trouble recreating the
original directory with all its properties such as ownership, permissions,
etc., at least not without making the code anything but simpler. There
might even be a situation were you are allowed to delete the directory but
cannot create a new one.

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

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Andrew Dunstan writes:
>> recreating the datadir if we didn't create it initially should be OK in
>> that case, and it makes the code simpler.

> That should be avoided, because you'll have trouble recreating the
> original directory with all its properties such as ownership, permissions,
> etc., at least not without making the code anything but simpler. There
> might even be a situation were you are allowed to delete the directory but
> cannot create a new one.

Consider also the strong likelihood that the data directory's parent
directory is owned by root, so that you do not have the ability to
delete and recreate the data directory because you don't have write
permission on its parent. The main reason initdb is set up to be able
to start with an existing-but-empty data dir is exactly because creating
that directory may have required permissions that initdb itself hasn't
got.

regards, tom lane

Bruce Momjian wrote:

>Andrew Dunstan wrote:
>
>
>>
>>
>>My comments:
>>
>>I have no problem with shelling out to rmdir - although my goal was to
>>avoid shelling out to anything other than postgres itself. I think
>>recreating the datadir if we didn't create it initially should be OK in
>>that case, and it makes the code simpler. Not handling dot files in the
>>Unix case should also be fine, as we know the directory was empty to
>>start with and we don't create any.
>>
>>
>
>Good. I can do rmdir() in C in port/dirmod.c if we need it. Right now
>we are doing system(rm/rmdir) in dbcommands.c so we should consistent.
>Let's stay with system(rm/rmdir) and if it doesn't work as we expect, we
>can add your rmdir() code and put it in port/dirmod.c.
>
>

In view of Peter's email of a few minutes ago I think we need to do that.

>>The changes you made for the final message are not going to work for
>>Windows - if pgpath or pg_data contain spaces we need quotes (even on
>>Unix, although there most people know better than to put spaces in
>>names). Also see above about mixed slashes. Also, putting a \ before
>>pg_data will be nasty if it starts with a drive or network specifier -
>>or even without (you might turn a directory specifier into a network
>>drive specifier). It's just a message, though, and we shouldn't hold up
>>for that - we can patch it to get it right.
>>
>>
>
>I am confused. The only change I made was to have the quotes appear
>only on WIN32.
>
> #ifndef WIN32
> #define QUOTE_PATH ""
> #else
> #define QUOTE_PATH "\""
> #endif
>
> ...
>
> printf("\n"
> "Success. You can now start the database server using:\n\n"
> " %s/postmaster -D %s%s%s\n"
> "or\n"
> " %s/pg_ctl -D %s%s%s -l logfile start\n\n",
> pgpath, QUOTE_PATH, pg_data, QUOTE_PATH,
> pgpath, QUOTE_PATH, pg_data, QUOTE_PATH);
>
>(I also merged multiple \n into a single line.) My logic was that
>spaces in directory names are much more common on WIN32, so we should
>display the quotes only on WIN32. Perhaps you read "\"" as "\\"?
>
>

Yes, I did. Sorry about that. But we also need to quote the path (the
most obvious place to put it after all is "C:\Program
Files\PostgreSQL"). In fact, that's more important than the data
location. Unfortunately, the Windows command processor barfs on multiple
quotes strings ;-(

cheers

andrew

Peter Eisentraut wrote:
> Andrew Dunstan writes:
>
> > recreating the datadir if we didn't create it initially should be OK in
> > that case, and it makes the code simpler.
>
> That should be avoided, because you'll have trouble recreating the
> original directory with all its properties such as ownership, permissions,
> etc., at least not without making the code anything but simpler. There
> might even be a situation were you are allowed to delete the directory but
> cannot create a new one.

Recreating the directory only happens on WIN32, where rmdir doesn't
allow you to only delete files and subdirectories and not the parent
directory. Non-Win32 does rm -rf dir/*.

Is that OK?

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

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > Andrew Dunstan writes:
> >> recreating the datadir if we didn't create it initially should be OK in
> >> that case, and it makes the code simpler.
>
> > That should be avoided, because you'll have trouble recreating the
> > original directory with all its properties such as ownership, permissions,
> > etc., at least not without making the code anything but simpler. There
> > might even be a situation were you are allowed to delete the directory but
> > cannot create a new one.
>
> Consider also the strong likelihood that the data directory's parent
> directory is owned by root, so that you do not have the ability to
> delete and recreate the data directory because you don't have write
> permission on its parent. The main reason initdb is set up to be able
> to start with an existing-but-empty data dir is exactly because creating
> that directory may have required permissions that initdb itself hasn't
> got.

Again, this directory recreate happens only on Win32, an I thought it
would be OK there.

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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>> Consider also the strong likelihood that the data directory's parent
>> directory is owned by root,

> Again, this directory recreate happens only on Win32, an I thought it
> would be OK there.

Windows has no concept of directory permissions at all? I thought the
more recent versions had at least rudimentary permissions.

regards, tom lane

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Recreating the directory only happens on WIN32, where rmdir doesn't
> allow you to only delete files and subdirectories and not the parent
> directory. Non-Win32 does rm -rf dir/*.

I think we should forget about invoking rm as a subprocess at all, and
just do the recursive directory walk and unlinks for ourselves. We
already have code to do this for copy in copydir.c, and unlink would not
be any longer. We will probably be forced into implementing database
removal for ourselves rather than by 'rm' hacks anyway as soon as
tablespaces come to pass; so why contort initdb's behavior for a very
transient implementation savings?

regards, tom lane

Andrew Dunstan wrote:
> >Good. I can do rmdir() in C in port/dirmod.c if we need it. Right now
> >we are doing system(rm/rmdir) in dbcommands.c so we should consistent.
> >Let's stay with system(rm/rmdir) and if it doesn't work as we expect, we
> >can add your rmdir() code and put it in port/dirmod.c.
> >
> >
>
> In view of Peter's email of a few minutes ago I think we need to do that.

Again, recreate is only Win32. I just figured out how to do this on
Win32. We have to use 'del' rather than 'rmdir' to keep the directory:

New code is:

/*
* delete a directory tree recursively
* assumes path points to a valid directory
* deletes everything under path
* if rmtopdir is true deletes the directory too
*
*/
static bool
rmtree(char *path, bool rmtopdir)
{
char buf[MAXPGPATH + 64];

#ifndef WIN32
/* doesn't handle .* files */
snprintf(buf, sizeof(buf), "rm -rf '%s%s'", path,
rmtopdir ? "" : "/*");
#else
snprintf(buf, sizeof(buf), "%s /s /q \"%s\"",
rmtopdir ? "rmdir" : "del", path);
#endif

return !system(buf);
}

> > printf("\n"
> > "Success. You can now start the database server using:\n\n"
> > " %s/postmaster -D %s%s%s\n"
> > "or\n"
> > " %s/pg_ctl -D %s%s%s -l logfile start\n\n",
> > pgpath, QUOTE_PATH, pg_data, QUOTE_PATH,
> > pgpath, QUOTE_PATH, pg_data, QUOTE_PATH);
> >
> >(I also merged multiple \n into a single line.) My logic was that
> >spaces in directory names are much more common on WIN32, so we should
> >display the quotes only on WIN32. Perhaps you read "\"" as "\\"?
> >
> >
>
> Yes, I did. Sorry about that. But we also need to quote the path (the
> most obvious place to put it after all is "C:\Program
> Files\PostgreSQL"). In fact, that's more important than the data
> location. Unfortunately, the Windows command processor barfs on multiple
> quotes strings ;-(

I ran some tests using XP "CMD" and found:

"C:\test"
and
"\test"

works but:

"test"

does not work. Since I see that the output always has a leading path, I
have modified the code to do:

printf("\nSuccess. You can now start the database server using:\n\n"
" %s%s%s/postmaster -D %s%s%s\n"
"or\n"
" %s%s%s/pg_ctl -D %s%s%s -l logfile start\n\n",
QUOTE_PATH, pgpath, QUOTE_PATH, QUOTE_PATH, pg_data, QUOTE_PATH,
QUOTE_PATH, pgpath, QUOTE_PATH, QUOTE_PATH, pg_data, QUOTE_PATH);

I am attaching the updated initdb.c file.

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Recreating the directory only happens on WIN32, where rmdir doesn't
> > allow you to only delete files and subdirectories and not the parent
> > directory. Non-Win32 does rm -rf dir/*.
>
> I think we should forget about invoking rm as a subprocess at all, and
> just do the recursive directory walk and unlinks for ourselves. We
> already have code to do this for copy in copydir.c, and unlink would not
> be any longer. We will probably be forced into implementing database
> removal for ourselves rather than by 'rm' hacks anyway as soon as
> tablespaces come to pass; so why contort initdb's behavior for a very
> transient implementation savings?

If we want to do that, fine, but I don't want to force the change just
for Win32.

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> >> Consider also the strong likelihood that the data directory's parent
> >> directory is owned by root,
>
> > Again, this directory recreate happens only on Win32, an I thought it
> > would be OK there.
>
> Windows has no concept of directory permissions at all? I thought the
> more recent versions had at least rudimentary permissions.

I found "del" works for what we need. I have posted the new code
already.

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

Tom Lane wrote:

>Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>
>
>>>Consider also the strong likelihood that the data directory's parent
>>>directory is owned by root,
>>>
>>>
>
>
>
>>Again, this directory recreate happens only on Win32, an I thought it
>>would be OK there.
>>
>>
>
>Windows has no concept of directory permissions at all? I thought the
>more recent versions had at least rudimentary permissions.
>

More than rudimentary on the server versions.

I found this info at http://www.cs.wayne.edu/labPages/modes.htm :

Windows ACLs

Windows NT and Windows 2000 uses Access Control Lists or ACLs to
determine what operations a user may perform on a file. Windows ACLs
allow one to set permissions with finer control that does the Unix file
mode. For example, one can all[ow] a user to append data to a file as
opposed to overwiting the file. ACLs also allow one to permit specific
users to change the permissions on a file. Perhaps the biggest
difference is that ACLs allow us to accord permissions on a user-by-user
basis, rather than the three categories of users permitted by Unix file
systems.

This info applies to directories as well as plain files AFAIK

cheers

andrew

Also, I see this at the top of the code:

* author: Andrew Dunstan mailto:andrew(at)dunslane(dot)net
*
* Copyright (C) 2003 Andrew Dunstan
* Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California

Can I remove the first copyright?

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

Bruce Momjian wrote:

>Also, I see this at the top of the code:
>
> * author: Andrew Dunstan mailto:andrew(at)dunslane(dot)net
> *
> * Copyright (C) 2003 Andrew Dunstan
> * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
> * Portions Copyright (c) 1994, Regents of the University of California
>
>Can I remove the first copyright?
>
>

Sure. I wasn't sure what our conventions were on that.

cheers

andrew

Bruce Momjian wrote:

>
>
>I ran some tests using XP "CMD" and found:
>
> "C:\test"
>and
> "\test"
>
>works but:
>
> "test"
>
>does not work. Since I see that the output always has a leading path,
>

On Windows, pgpath is guaranteed to be a full path (see call to
expanded_path() ) exactly so it works inside quotes using the Windows
command processor.

> I
>have modified the code to do:
>
> printf("\nSuccess. You can now start the database server using:\n\n"
> " %s%s%s/postmaster -D %s%s%s\n"
> "or\n"
> " %s%s%s/pg_ctl -D %s%s%s -l logfile start\n\n",
> QUOTE_PATH, pgpath, QUOTE_PATH, QUOTE_PATH, pg_data, QUOTE_PATH,
> QUOTE_PATH, pgpath, QUOTE_PATH, QUOTE_PATH, pg_data, QUOTE_PATH);
>
>I am attaching the updated initdb.c file.
>
>
>

The problem with this is that you now have 2 quoted strings. This is
exactly the problem that I solved inside initdb by passing pg_data via
the environment rather than on the command line. "help cmd" on XP gives
you this info:

If /C or /K is specified, then the remainder of the command line after
the switch is processed as a command line, where the following logic is
used to process quote (") characters:

1. If all of the following conditions are met, then quote characters
on the command line are preserved:

- no /S switch
- exactly two quote characters
- no special characters between the two quote characters,
where special is one of: &<>()@^|
- there are one or more whitespace characters between the
the two quote characters
- the string between the two quote characters is the name
of an executable file.

2. Otherwise, old behavior is to see if the first character is
a quote character and if so, strip the leading character and
remove the last quote character on the command line, preserving
any text after the last quote character.

It is amazingly brain dead and cost me hours and hours of grief trying
to work out WTF was going on.

Offhand I can't think of a simple guaranteed to work command line for
the Windows message.

In the last resort we might need to look at having initdb create a .bat
file or two for us.

:-(((

cheers

andrew

Andrew Dunstan wrote:

>
>
>
> The problem with this is that you now have 2 quoted strings.

I take this back. You *can* have multiple quoted strings on the command
line - it just doesn't work in popen() because it uses the /C flag.

Sorry for my confusion.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Andrew Dunstan wrote:
>
> >
> >
> >
> > The problem with this is that you now have 2 quoted strings.
>
>
> I take this back. You *can* have multiple quoted strings on the command
> line - it just doesn't work in popen() because it uses the /C flag.

Oh, OK, that's good.

--
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. We now have a C version of initdb!

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

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
>
> >Here is a slightly modified version of Andrew's great work in making a C
> >version of initdb. Other than minor cleanups, the only big change was
> >to remove rmdir handling because we using rm -r and rmdir /s in
> >commands/dbcommands.c, so we might as use the same thing for initdb.c
> >rather than having code that traverses the directory tree doing 'rm'.
> >
> >The other change was to remove the unlink code and instead use
> >port/dirmod.c's version.
> >
> >It passes all the regression tests. I have also included a diff against
> >Andrew's version so you can see my changes. It seems Andrew had a very
> >current version of initdb. The only update he missed was the change to
> >test the number of connections before shared buffers --- I made that
> >change myself.
> >
> >I would like to apply this in the next few days to HEAD before initdb.sh
> >drifts. We are no longer using the WIN32_DEV branch because all our
> >work is now in 7.5 head.
> >
> >
>
> My comments:
>
> I have no problem with shelling out to rmdir - although my goal was to
> avoid shelling out to anything other than postgres itself. I think
> recreating the datadir if we didn't create it initially should be OK in
> that case, and it makes the code simpler. Not handling dot files in the
> Unix case should also be fine, as we know the directory was empty to
> start with and we don't create any.
>
> Regarding the #endif comments you removed - Peter had asked me to put
> them in. See
> http://archives.postgresql.org/pgsql-hackers/2003-10/msg00352.php
>
> You put a comment on canonicalise_path() asking if it is needed. The
> short answer is very much "yes". The Windows command processor will
> accept suitably quoted paths with forward slashes, but barfs badly with
> mixed forward and back slashes. (See recent discussion on
> hackers-win32).Removing the trailing slash on a path means we never get
> ugly double slashes. Feel free to put that info in as a comment if you
> think it is needed.
>
> The changes you made for the final message are not going to work for
> Windows - if pgpath or pg_data contain spaces we need quotes (even on
> Unix, although there most people know better than to put spaces in
> names). Also see above about mixed slashes. Also, putting a \ before
> pg_data will be nasty if it starts with a drive or network specifier -
> or even without (you might turn a directory specifier into a network
> drive specifier). It's just a message, though, and we shouldn't hold up
> for that - we can patch it to get it right.
>
> (Getting the path and slash thing working portably was by far the
> biggest headache in this project - the rest was just grunt work for the
> most part, although I learned a few interesting things.)
>
> Otherwise I'm fine with this.
>
> cheers
>
> andrew
>
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>

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

What's become of initdb's initial note about who you are?

$ initdb
The files belonging to this database system will be owned by user "postgres".
This user must also own the server process.
... etc etc...

The first two lines of printout don't seem to be in the "highly
compatible" C version.

regards, tom lane

Apologies. Here's the patch. There may be other very minor differences
in wording, too. But if that's all I missed I won't think I did too
badly :-)

cheers

andrew

Tom Lane wrote:

>What's become of initdb's initial note about who you are?
>
>$ initdb
>The files belonging to this database system will be owned by user "postgres".
>This user must also own the server process.
>... etc etc...
>
>The first two lines of printout don't seem to be in the "highly
>compatible" C version.
>
>
>

ARRGGHH! It has a typo!. new patch shortly.

Andrew Dunstan wrote:

> Apologies. Here's the patch. There may be other very minor differences
> in wording, too. But if that's all I missed I won't think I did too
> badly :-)
>
> cheers
>
> andrew
>
> Tom Lane wrote:
>
>> What's become of initdb's initial note about who you are?
>>
>> $ initdb
>> The files belonging to this database system will be owned by user
>> "postgres".
>> This user must also own the server process.
>> ... etc etc...
>>
>> The first two lines of printout don't seem to be in the "highly
>> compatible" C version.
>>
>>
>>
>
>
>

OK, this works.

cheers

andrew

Andrew Dunstan wrote:

>
> ARRGGHH! It has a typo!. new patch shortly.
>
> Andrew Dunstan wrote:
>
>> Apologies. Here's the patch. There may be other very minor
>> differences in wording, too. But if that's all I missed I won't think
>> I did too badly :-)
>>
>> cheers
>>
>> andrew
>>
>> Tom Lane wrote:
>>
>>> What's become of initdb's initial note about who you are?
>>>
>>> $ initdb
>>> The files belonging to this database system will be owned by user
>>> "postgres".
>>> This user must also own the server process.
>>> ... etc etc...
>>>
>>> The first two lines of printout don't seem to be in the "highly
>>> compatible" C version.
>>>

Note to self: stop working when brain is clearly malfunctioning.

*This* patch works. Please disregard previous.

thanks

andrew

Andrew Dunstan wrote:

>
> OK, this works.
>
> cheers
>
> andrew
>
> Andrew Dunstan wrote:
>
>>
>> ARRGGHH! It has a typo!. new patch shortly.
>>
>> Andrew Dunstan wrote:
>>
>

Patch applied. Thanks.

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

Andrew Dunstan wrote:
> Note to self: stop working when brain is clearly malfunctioning.
>
> *This* patch works. Please disregard previous.
>
> thanks
>
> andrew
>
> Andrew Dunstan wrote:
>
> >
> > OK, this works.
> >
> > cheers
> >
> > andrew
> >
> > Andrew Dunstan wrote:
> >
> >>
> >> ARRGGHH! It has a typo!. new patch shortly.
> >>
> >> Andrew Dunstan wrote:
> >>
> >

> ? initdb
> Index: initdb.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/bin/initdb/initdb.c,v
> retrieving revision 1.4
> diff -c -w -r1.4 initdb.c
> *** initdb.c 13 Nov 2003 01:36:00 -0000 1.4
> --- initdb.c 13 Nov 2003 12:52:21 -0000
> ***************
> *** 2275,2280 ****
> --- 2275,2285 ----
> check_input(features_file);
> check_input(system_views_file);
>
> + printf("The files belonging to this database system will be owned "
> + "by user \"%s\".\n"
> + "This user must also own the server process.\n\n",
> + effective_user);
> +
> setlocales();
>
> if (strcmp(lc_ctype, lc_collate) == 0 &&

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org

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