Re: initdb in C

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