On Fri, Nov 30, 2012 at 06:06:39PM -0500, Andrew Dunstan wrote:
>
> On 11/30/2012 04:45 PM, Bruce Momjian wrote:
> >On Thu, Nov 29, 2012 at 11:12:23PM -0500, Bruce Momjian wrote:
> >>In looking to add an fsync-only option to initdb, I found its main()
> >>function to be 743 lines long, and very hard to understand.
> >>
> >>The attached patch moves much of that code into separate functions,
> >>which will make initdb.c easier to understand, and easier to add an
> >>fsync-only option. The original initdb.c author, Andrew Dunstan, has
> >>accepted the restructuring, in principle.
> >Applied.
> >
>
> Sorry I didn't have time to review this before it was applied.
>
> A few minor nitpicks:
>
> * process() is a fairly uninformative function name, not sure what I'd
> call it, but something more descriptive.
> * the setup_signals_and_umask() call and possibly the final message
> section of process() would be better placed back in main() IMNSHO.
> * the large statements for setting up the datadir and the xlogdir
> should be factored out of process() into their own functions, I
> think. That would make it much more readable.
Done with the attached patch. I kept the signals in their own function,
but moved the umask() call out --- I was not happy mixing those either.
--
Bruce Momjian <br...@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
commit 26374f2a0fc02b76a91b7565e908dbae99a3b5f9
Author: Bruce Momjian <br...@momjian.us>
Date: Mon Dec 3 23:22:56 2012 -0500
In initdb.c, rename some newly created functions, and move the directory
creation and xlog symlink creation to separate functions.
Per suggestions from Andrew Dunstan.
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
new file mode 100644
index 8c0a9f4..d44281b
*** a/src/bin/initdb/initdb.c
--- b/src/bin/initdb/initdb.c
*************** void setup_pgdata(void);
*** 255,263 ****
void setup_bin_paths(const char *argv0);
void setup_data_file_paths(void);
void setup_locale_encoding(void);
! void setup_signals_and_umask(void);
void setup_text_search(void);
! void process(const char *argv0);
#ifdef WIN32
--- 255,265 ----
void setup_bin_paths(const char *argv0);
void setup_data_file_paths(void);
void setup_locale_encoding(void);
! void setup_signals(void);
void setup_text_search(void);
! void create_data_directory(void);
! void create_xlog_symlink(void);
! void initialize_data_directory(void);
#ifdef WIN32
*************** setup_text_search(void)
*** 3164,3170 ****
void
! setup_signals_and_umask(void)
{
/* some of these are not valid on Windows */
#ifdef SIGHUP
--- 3166,3172 ----
void
! setup_signals(void)
{
/* some of these are not valid on Windows */
#ifdef SIGHUP
*************** setup_signals_and_umask(void)
*** 3184,3202 ****
#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
#endif
-
- umask(S_IRWXG | S_IRWXO);
}
void
! process(const char *argv0)
{
- int i;
- char bin_dir[MAXPGPATH];
-
- setup_signals_and_umask();
-
switch (pg_check_dir(pg_data))
{
case 0:
--- 3186,3197 ----
#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
#endif
}
void
! create_data_directory(void)
{
switch (pg_check_dir(pg_data))
{
case 0:
*************** process(const char *argv0)
*** 3249,3255 ****
--- 3244,3255 ----
progname, pg_data, strerror(errno));
exit_nicely();
}
+ }
+
+ void
+ create_xlog_symlink(void)
+ {
/* Create transaction log symlink, if required */
if (strcmp(xlog_dir, "") != 0)
{
*************** process(const char *argv0)
*** 3336,3341 ****
--- 3336,3356 ----
exit_nicely();
#endif
}
+ }
+
+
+ void
+ initialize_data_directory(void)
+ {
+ int i;
+
+ setup_signals();
+
+ umask(S_IRWXG | S_IRWXO);
+
+ create_data_directory();
+
+ create_xlog_symlink();
/* Create required subdirectories */
printf(_("creating subdirectories ... "));
*************** process(const char *argv0)
*** 3404,3422 ****
if (authwarning != NULL)
fprintf(stderr, "%s", authwarning);
-
- /* Get directory specification used to start this executable */
- strlcpy(bin_dir, argv0, sizeof(bin_dir));
- get_parent_directory(bin_dir);
-
- printf(_("\nSuccess. You can now start the database server using:\n\n"
- " %s%s%spostgres%s -D %s%s%s\n"
- "or\n"
- " %s%s%spg_ctl%s -D %s%s%s -l logfile start\n\n"),
- QUOTE_PATH, bin_dir, (strlen(bin_dir) > 0) ? DIR_SEP : "", QUOTE_PATH,
- QUOTE_PATH, pgdata_native, QUOTE_PATH,
- QUOTE_PATH, bin_dir, (strlen(bin_dir) > 0) ? DIR_SEP : "", QUOTE_PATH,
- QUOTE_PATH, pgdata_native, QUOTE_PATH);
}
--- 3419,3424 ----
*************** main(int argc, char *argv[])
*** 3459,3464 ****
--- 3461,3467 ----
int c;
int option_index;
char *effective_user;
+ char bin_dir[MAXPGPATH];
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("initdb"));
*************** main(int argc, char *argv[])
*** 3642,3648 ****
printf("\n");
! process(argv[0]);
return 0;
}
--- 3645,3664 ----
printf("\n");
! initialize_data_directory();
+ /* Get directory specification used to start this executable */
+ strlcpy(bin_dir, argv[0], sizeof(bin_dir));
+ get_parent_directory(bin_dir);
+
+ printf(_("\nSuccess. You can now start the database server using:\n\n"
+ " %s%s%spostgres%s -D %s%s%s\n"
+ "or\n"
+ " %s%s%spg_ctl%s -D %s%s%s -l logfile start\n\n"),
+ QUOTE_PATH, bin_dir, (strlen(bin_dir) > 0) ? DIR_SEP : "", QUOTE_PATH,
+ QUOTE_PATH, pgdata_native, QUOTE_PATH,
+ QUOTE_PATH, bin_dir, (strlen(bin_dir) > 0) ? DIR_SEP : "", QUOTE_PATH,
+ QUOTE_PATH, pgdata_native, QUOTE_PATH);
+
return 0;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
