Thread: GUC patch for Win32
Here is a patch that saves the postmaster GUC state into a file to be read in by exec'ed backends. This is a start toward a fork/exec option for Unix (for testing) and a CreateProcess option for Win32. The patch basically writes all modified GUC variables to a binary file that can be read in by newly created exec'ed backends. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/backend/postmaster/postmaster.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/postmaster/postmaster.c,v retrieving revision 1.315 diff -c -c -r1.315 postmaster.c *** src/backend/postmaster/postmaster.c 26 Apr 2003 02:57:14 -0000 1.315 --- src/backend/postmaster/postmaster.c 1 May 2003 19:53:54 -0000 *************** *** 256,266 **** static void CleanupProc(int pid, int exitstatus); static void LogChildExit(int lev, const char *procname, int pid, int exitstatus); ! static int DoBackend(Port *port); void ExitPostmaster(int status); static void usage(const char *); static int ServerLoop(void); static int BackendStartup(Port *port); static int ProcessStartupPacket(Port *port, bool SSLdone); static void processCancelRequest(Port *port, void *pkt); static int initMasks(fd_set *rmask, fd_set *wmask); --- 256,267 ---- static void CleanupProc(int pid, int exitstatus); static void LogChildExit(int lev, const char *procname, int pid, int exitstatus); ! static int BackendFinalize(Port *port); void ExitPostmaster(int status); static void usage(const char *); static int ServerLoop(void); static int BackendStartup(Port *port); + static void BackendFork(Port *port, Backend *bn); static int ProcessStartupPacket(Port *port, bool SSLdone); static void processCancelRequest(Port *port, void *pkt); static int initMasks(fd_set *rmask, fd_set *wmask); *************** *** 570,575 **** --- 571,579 ---- SetDataDir(potential_DataDir); ProcessConfigFile(PGC_POSTMASTER); + #ifdef EXEC_BACKEND + write_nondefault_variables(PGC_POSTMASTER); + #endif /* * Check for invalid combinations of GUC settings. *************** *** 1231,1237 **** * Now fetch parameters out of startup packet and save them into the * Port structure. All data structures attached to the Port struct * must be allocated in TopMemoryContext so that they won't disappear ! * when we pass them to PostgresMain (see DoBackend). We need not worry * about leaking this storage on failure, since we aren't in the postmaster * process anymore. */ --- 1235,1241 ---- * Now fetch parameters out of startup packet and save them into the * Port structure. All data structures attached to the Port struct * must be allocated in TopMemoryContext so that they won't disappear ! * when we pass them to PostgresMain (see BackendFinalize). We need not worry * about leaking this storage on failure, since we aren't in the postmaster * process anymore. */ *************** *** 1568,1573 **** --- 1572,1580 ---- elog(LOG, "Received SIGHUP, reloading configuration files"); SignalChildren(SIGHUP); ProcessConfigFile(PGC_SIGHUP); + #ifdef EXEC_BACKEND + write_nondefault_variables(PGC_SIGHUP); + #endif load_hba(); load_ident(); } *************** *** 2053,2080 **** pid = fork(); if (pid == 0) /* child */ ! { ! int status; ! ! #ifdef LINUX_PROFILE ! setitimer(ITIMER_PROF, &prof_itimer, NULL); ! #endif ! ! #ifdef __BEOS__ ! /* Specific beos backend startup actions */ ! beos_backend_startup(); ! #endif ! free(bn); ! ! status = DoBackend(port); ! if (status != 0) ! { ! elog(LOG, "connection startup failed"); ! proc_exit(status); ! } ! else ! proc_exit(0); ! } /* in parent, error */ if (pid < 0) --- 2060,2066 ---- pid = fork(); if (pid == 0) /* child */ ! BackendFork(port, bn); /* never returns */ /* in parent, error */ if (pid < 0) *************** *** 2108,2113 **** --- 2094,2124 ---- } + static void + BackendFork(Port *port, Backend *bn) + { + int status; + + #ifdef LINUX_PROFILE + setitimer(ITIMER_PROF, &prof_itimer, NULL); + #endif + + #ifdef __BEOS__ + /* Specific beos backend startup actions */ + beos_backend_startup(); + #endif + free(bn); + + status = BackendFinalize(port); + if (status != 0) + { + elog(LOG, "connection startup failed"); + proc_exit(status); + } + else + proc_exit(0); + } + /* * Try to report backend fork() failure to client before we close the * connection. Since we do not care to risk blocking the postmaster on *************** *** 2173,2179 **** } /* ! * DoBackend -- perform authentication, and if successful, set up the * backend's argument list and invoke backend main(). * * This used to perform an execv() but we no longer exec the backend; --- 2184,2190 ---- } /* ! * BackendFinalize -- perform authentication, and if successful, set up the * backend's argument list and invoke backend main(). * * This used to perform an execv() but we no longer exec the backend; *************** *** 2184,2190 **** * If PostgresMain() fails, return status. */ static int ! DoBackend(Port *port) { char *remote_host; char **av; --- 2195,2201 ---- * If PostgresMain() fails, return status. */ static int ! BackendFinalize(Port *port) { char *remote_host; char **av; *************** *** 2221,2226 **** --- 2232,2241 ---- /* Reset MyProcPid to new backend's pid */ MyProcPid = getpid(); + #ifdef EXEC_BACKEND + read_nondefault_variables(); + #endif + /* * Initialize libpq and enable reporting of elog errors to the client. * Must do this now because authentication uses libpq to send *************** *** 2248,2254 **** unsigned short remote_port; char *host_addr; #ifdef HAVE_IPV6 ! char ip_hostinfo[INET6_ADDRSTRLEN]; #else char ip_hostinfo[INET_ADDRSTRLEN]; #endif --- 2263,2269 ---- unsigned short remote_port; char *host_addr; #ifdef HAVE_IPV6 ! char ip_hostinfo[INET6_ADDRSTRLEN]; #else char ip_hostinfo[INET_ADDRSTRLEN]; #endif *************** *** 2294,2300 **** } else { ! /* not AF_INET */ remote_host = "[local]"; if (Log_connections) --- 2309,2315 ---- } else { ! /* not AF_INET */ remote_host = "[local]"; if (Log_connections) *************** *** 2318,2324 **** * indefinitely. PreAuthDelay doesn't count against the time limit. */ if (!enable_sig_alarm(AuthenticationTimeout * 1000, false)) ! elog(FATAL, "DoBackend: Unable to set timer for auth timeout"); /* * Receive the startup packet (which might turn out to be a cancel --- 2333,2339 ---- * indefinitely. PreAuthDelay doesn't count against the time limit. */ if (!enable_sig_alarm(AuthenticationTimeout * 1000, false)) ! elog(FATAL, "BackendFinalize: Unable to set timer for auth timeout"); /* * Receive the startup packet (which might turn out to be a cancel *************** *** 2347,2353 **** * SIGTERM/SIGQUIT again until backend startup is complete. */ if (!disable_sig_alarm(false)) ! elog(FATAL, "DoBackend: Unable to disable timer for auth timeout"); PG_SETMASK(&BlockSig); if (Log_connections) --- 2362,2368 ---- * SIGTERM/SIGQUIT again until backend startup is complete. */ if (!disable_sig_alarm(false)) ! elog(FATAL, "BackendFinalize: Unable to disable timer for auth timeout"); PG_SETMASK(&BlockSig); if (Log_connections) Index: src/backend/utils/misc/guc.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/utils/misc/guc.c,v retrieving revision 1.119 diff -c -c -r1.119 guc.c *** src/backend/utils/misc/guc.c 25 Apr 2003 19:45:09 -0000 1.119 --- src/backend/utils/misc/guc.c 1 May 2003 19:53:57 -0000 *************** *** 60,65 **** --- 60,68 ---- #define PG_KRB_SRVTAB "" #endif + #ifdef EXEC_BACKEND + #define CONFIG_EXEC_PARAMS "global/config_exec_params" + #endif /* XXX these should appear in other modules' header files */ extern bool Log_connections; *************** *** 2801,2806 **** --- 2804,3007 ---- } + #ifdef EXEC_BACKEND + /* + * This routine dumps out all non-default GUC options into a binary + * file that is read by all exec'ed backends. The format is: + * + * variable name, string, null terminated + * variable value, string, null terminated + * variable source, integer + */ + void + write_nondefault_variables(GucContext context) + { + int i; + char *new_filename, *filename; + int elevel; + FILE *fp; + + Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP); + Assert(DataDir); + elevel = (context == PGC_SIGHUP) ? DEBUG3 : ERROR; + + /* + * Open file + */ + new_filename = malloc(strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) + + strlen(".new") + 2); + filename = malloc(strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) + 2); + if (new_filename == NULL || filename == NULL) + { + elog(elevel, "out of memory"); + return; + } + sprintf(new_filename, "%s/" CONFIG_EXEC_PARAMS ".new", DataDir); + sprintf(filename, "%s/" CONFIG_EXEC_PARAMS, DataDir); + + fp = AllocateFile(new_filename, "w"); + if (!fp) + { + free(new_filename); + free(filename); + elog(elevel, "could not write exec config params file `" + CONFIG_EXEC_PARAMS "': %s", strerror(errno)); + return; + } + + for (i = 0; i < num_guc_variables; i++) + { + struct config_generic *gconf = guc_variables[i]; + + if (gconf->source != PGC_S_DEFAULT) + { + fprintf(fp, "%s", gconf->name); + fputc(0, fp); + + switch (gconf->vartype) + { + case PGC_BOOL: + { + struct config_bool *conf = (struct config_bool *) gconf; + + if (*conf->variable == 0) + fprintf(fp, "false"); + else + fprintf(fp, "true"); + } + break; + + case PGC_INT: + { + struct config_int *conf = (struct config_int *) gconf; + + fprintf(fp, "%d", *conf->variable); + } + break; + + case PGC_REAL: + { + struct config_real *conf = (struct config_real *) gconf; + + /* Could lose precision here? */ + fprintf(fp, "%f", *conf->variable); + } + break; + + case PGC_STRING: + { + struct config_string *conf = (struct config_string *) gconf; + + fprintf(fp, "%s", *conf->variable); + } + break; + } + + fputc(0, fp); + + fwrite(&gconf->source, sizeof(gconf->source), 1, fp); + } + } + + FreeFile(fp); + /* Put new file in place, this could delay on Win32 */ + rename(new_filename, filename); + free(new_filename); + free(filename); + return; + } + + + /* + * Read string, including null byte from file + * + * Return NULL on EOF and nothing read + */ + static char * + read_string_with_null(FILE *fp) + { + int i = 0, ch, maxlen = 256; + char *str = NULL; + + do + { + if ((ch = fgetc(fp)) == EOF) + { + if (i == 0) + return NULL; + else + elog(FATAL, "Invalid format of exec config params file"); + } + if (i == 0) + str = malloc(maxlen); + else if (i == maxlen) + str = realloc(str, maxlen *= 2); + str[i++] = ch; + } while (ch != 0); + + return str; + } + + + /* + * This routine loads a previous postmaster dump of its non-default + * settings. + */ + void + read_nondefault_variables(void) + { + char *filename; + FILE *fp; + char *varname, *varvalue; + int varsource; + + Assert(DataDir); + + /* + * Open file + */ + filename = malloc(strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) + 2); + if (filename == NULL) + { + elog(ERROR, "out of memory"); + return; + } + sprintf(filename, "%s/" CONFIG_EXEC_PARAMS, DataDir); + + fp = AllocateFile(filename, "r"); + if (!fp) + { + free(filename); + /* File not found is fine */ + if (errno != ENOENT) + elog(FATAL, "could not read exec config params file `" + CONFIG_EXEC_PARAMS "': %s", strerror(errno)); + return; + } + + while (1) + { + if ((varname = read_string_with_null(fp)) == NULL) + break; + + if ((varvalue = read_string_with_null(fp)) == NULL) + elog(FATAL, "Invalid format of exec config params file"); + if (fread(&varsource, sizeof(varsource), 1, fp) == 0) + elog(FATAL, "Invalid format of exec config params file"); + + (void) set_config_option(varname, varvalue, PGC_POSTMASTER, + varsource, false, true); + free(varname); + free(varvalue); + } + + FreeFile(fp); + free(filename); + return; + } + #endif + + /* * A little "long argument" simulation, although not quite GNU * compliant. Takes a string of the form "some-option=some value" and *************** *** 3203,3205 **** --- 3404,3407 ---- #include "guc-file.c" + Index: src/include/utils/guc.h =================================================================== RCS file: /cvsroot/pgsql-server/src/include/utils/guc.h,v retrieving revision 1.27 diff -c -c -r1.27 guc.h *** src/include/utils/guc.h 25 Apr 2003 19:45:09 -0000 1.27 --- src/include/utils/guc.h 1 May 2003 19:54:01 -0000 *************** *** 135,138 **** --- 135,143 ---- extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value); extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name); + #ifdef EXEC_BACKEND + void write_nondefault_variables(GucContext context); + void read_nondefault_variables(void); + #endif + #endif /* GUC_H */
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The patch basically writes all modified GUC variables to a binary file
> that can be read in by newly created exec'ed backends.
Where exactly is the interlock to ensure that the new backend will end up
with the correct settings if someone is changing the values at about
the time of the fork?
The relcache code has this problem solved, but I'm unconvinced that GUC
does.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The patch basically writes all modified GUC variables to a binary file > > that can be read in by newly created exec'ed backends. > > Where exactly is the interlock to ensure that the new backend will end up > with the correct settings if someone is changing the values at about > the time of the fork? Postmaster creates a new file, then does rename() to move it to the name used by the backends. It can't move it until the file is not in use. > The relcache code has this problem solved, but I'm unconvinced that GUC > does. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
The other nice thing is that it only creates the file on startup and on sighup, so it obeys the normal Unix behavior. --------------------------------------------------------------------------- Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > The patch basically writes all modified GUC variables to a binary file > > that can be read in by newly created exec'ed backends. > > Where exactly is the interlock to ensure that the new backend will end up > with the correct settings if someone is changing the values at about > the time of the fork? > > The relcache code has this problem solved, but I'm unconvinced that GUC > does. > > regards, tom lane > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.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@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> Where exactly is the interlock to ensure that the new backend will end up
>> with the correct settings if someone is changing the values at about
>> the time of the fork?
> Postmaster creates a new file, then does rename() to move it to the name
> used by the backends. It can't move it until the file is not in use.
And?
How exactly does that guarantee that the new backend will see an update
occurring at about the same time? I'm pretty sure that GUC is fired up
before backends start listening to signals (and that's assuming the
Windows port has a Unixy idea of signal response, which I seem to recall
you telling me wasn't the case).
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> Where exactly is the interlock to ensure that the new backend will end up > >> with the correct settings if someone is changing the values at about > >> the time of the fork? > > > Postmaster creates a new file, then does rename() to move it to the name > > used by the backends. It can't move it until the file is not in use. > > And? > > How exactly does that guarantee that the new backend will see an update > occurring at about the same time? I'm pretty sure that GUC is fired up > before backends start listening to signals (and that's assuming the > Windows port has a Unixy idea of signal response, which I seem to recall > you telling me wasn't the case). Oh, I am not sure. I haven't gotten the signal stuff done yet. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Applied. I have added a note to check for file changes before signal handler is installed in child. --------------------------------------------------------------------------- Bruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > > Tom Lane wrote: > > >> Where exactly is the interlock to ensure that the new backend will end up > > >> with the correct settings if someone is changing the values at about > > >> the time of the fork? > > > > > Postmaster creates a new file, then does rename() to move it to the name > > > used by the backends. It can't move it until the file is not in use. > > > > And? > > > > How exactly does that guarantee that the new backend will see an update > > occurring at about the same time? I'm pretty sure that GUC is fired up > > before backends start listening to signals (and that's assuming the > > Windows port has a Unixy idea of signal response, which I seem to recall > > you telling me wasn't the case). > > > Oh, I am not sure. I haven't gotten the signal stuff done yet. > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.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 4: Don't 'kill -9' the postmaster > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
What about all the other global variables? Will there be a separate mechanism for each kind? Bruce Momjian writes: > Here is a patch that saves the postmaster GUC state into a file to be > read in by exec'ed backends. This is a start toward a fork/exec option > for Unix (for testing) and a CreateProcess option for Win32. > > The patch basically writes all modified GUC variables to a binary file > that can be read in by newly created exec'ed backends. -- Peter Eisentraut peter_e@gmx.net
Peter Eisentraut wrote: > What about all the other global variables? Will there be a separate > mechanism for each kind? Good question. I am not sure yet. I figured I would hit the GUC ones first because they are easy and all in one place, then see what others exist in the Peer Direct patch. Did I hit all the GUC structure members that need to be dumped --- name, value, source? > > Bruce Momjian writes: > > > Here is a patch that saves the postmaster GUC state into a file to be > > read in by exec'ed backends. This is a start toward a fork/exec option > > for Unix (for testing) and a CreateProcess option for Win32. > > > > The patch basically writes all modified GUC variables to a binary file > > that can be read in by newly created exec'ed backends. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.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 writes: > Peter Eisentraut wrote: > > What about all the other global variables? Will there be a separate > > mechanism for each kind? > > Good question. I am not sure yet. I figured I would hit the GUC ones > first because they are easy and all in one place, then see what others > exist in the Peer Direct patch. Well, the question of what to do with all the global variables seems to be a central point in the port, given that no fork() function exists. So I think that should be solved before piecewise solutions for some variables are installed that will become obsolete later on. -- Peter Eisentraut peter_e@gmx.net
Peter Eisentraut wrote: > Bruce Momjian writes: > > > Peter Eisentraut wrote: > > > What about all the other global variables? Will there be a separate > > > mechanism for each kind? > > > > Good question. I am not sure yet. I figured I would hit the GUC ones > > first because they are easy and all in one place, then see what others > > exist in the Peer Direct patch. > > Well, the question of what to do with all the global variables seems to be > a central point in the port, given that no fork() function exists. So > I think that should be solved before piecewise solutions for some > variables are installed that will become obsolete later on. Well, I am pretty far along and haven't seen tons of problems with globals yet --- most of the globals don't pass from postmaster to backend, I guess. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.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@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> Where exactly is the interlock to ensure that the new backend will end up > >> with the correct settings if someone is changing the values at about > >> the time of the fork? > > > Postmaster creates a new file, then does rename() to move it to the name > > used by the backends. It can't move it until the file is not in use. > > And? > > How exactly does that guarantee that the new backend will see an update > occurring at about the same time? I'm pretty sure that GUC is fired up > before backends start listening to signals (and that's assuming the > Windows port has a Unixy idea of signal response, which I seem to recall > you telling me wasn't the case). So far the postmaster is not multi-threaded, so it will not create a new file and start a backend at the same time. Also, the rename() call is supposed to be atomic. So there is allways a file, and it's either the old or the new one, never something in between. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck <JanWieck@Yahoo.com> writes:
> So far the postmaster is not multi-threaded, so it will not create a new
> file and start a backend at the same time. Also, the rename() call is
> supposed to be atomic. So there is allways a file, and it's either the
> old or the new one, never something in between.
(a) rename isn't atomic on Windows, I thought.
(b) I'm still not convinced that there's no race condition in SIGHUP.
(It doesn't help that SIGHUP_handler does SignalChildren() *before*
reading the file for itself --- that looks wrong.)
It occurs to me though that there need be no problem because the only
GUC variables that the postmaster really needs to send to children
are the ones it gets from its command line and environment. Updates
coming from postgresql.conf are not a problem because the children
can get them for themselves (as already-launched backends surely must).
Thus, write_nondefault_variables is misdesigned: it should be called
*once*, not during SIGHUP_handler, and should only be responsible for
writing out values that came from PGC_S_ENV_VAR or PGC_S_ARGV sources
(maybe also PGC_S_OVERRIDE, not sure if postmaster startup uses that).
That way the file never changes after postmaster start and there can
be no race condition. Children will instead have to read
postgresql.conf for themselves during their launch (after they read the
nondefault_variables file), but that's an easy one-line addition.
regards, tom lane
I said:
> That way the file never changes after postmaster start and there can
> be no race condition. Children will instead have to read
> postgresql.conf for themselves during their launch (after they read the
> nondefault_variables file), but that's an easy one-line addition.
On second thought, that just moves the race condition upstream (to the
person editing postgresql.conf) ... so never mind that idea. We don't
want any part of Postgres reading postgresql.conf except shortly after
someone has SIGHUP'd the postmaster.
But I'm still wondering if the order of operations in SIGHUP_handler is
wrong.
regards, tom lane
Tom Lane wrote: > > I said: > > That way the file never changes after postmaster start and there can > > be no race condition. Children will instead have to read > > postgresql.conf for themselves during their launch (after they read the > > nondefault_variables file), but that's an easy one-line addition. > > On second thought, that just moves the race condition upstream (to the > person editing postgresql.conf) ... so never mind that idea. We don't > want any part of Postgres reading postgresql.conf except shortly after > someone has SIGHUP'd the postmaster. I didn't actually look at that part of the code yet ... why would any backend read postgresql.conf? I thought only the postmaster does that and that he SIGHUP's all backends after writing the new file. That way there is not race condition other that hupping the PM while w'ing the file. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck <JanWieck@Yahoo.com> writes:
> Tom Lane wrote:
>> On second thought, that just moves the race condition upstream (to the
>> person editing postgresql.conf) ... so never mind that idea.
> I didn't actually look at that part of the code yet ... why would any
> backend read postgresql.conf?
At SIGHUP, the postmaster re-reads postgresql.conf, and each backend
must do so too --- there's no other way for pre-existing backends to
learn about changed values. The assumption is that this happens over a
short enough timespan that it's okay from the perspective of the person
editing postgresql.conf. But if newly started backends were to read
postgresql.conf when they start, that would be bad news for someone
trying to edit postgresql.conf, because there'd be no way to know when
it would happen.
regards, tom lane
Tom Lane wrote: > > Jan Wieck <JanWieck@Yahoo.com> writes: > > Tom Lane wrote: > >> On second thought, that just moves the race condition upstream (to the > >> person editing postgresql.conf) ... so never mind that idea. > > > I didn't actually look at that part of the code yet ... why would any > > backend read postgresql.conf? > > At SIGHUP, the postmaster re-reads postgresql.conf, and each backend > must do so too --- there's no other way for pre-existing backends to > learn about changed values. The assumption is that this happens over a > short enough timespan that it's okay from the perspective of the person > editing postgresql.conf. But if newly started backends were to read > postgresql.conf when they start, that would be bad news for someone > trying to edit postgresql.conf, because there'd be no way to know when > it would happen. Sure is there ... the way I did it originally. The postmaster reads the config file and overrides with commandline options. Then he dumps ALL options into a separate file. A backend does not need to read the config file or parse commandline at all. It just jeads the postmaster provided file. If the user changes the config and HUP's the postmaster, postmaster rereads the config and merges only those changes, that are changable at runtime into it's status. From that status it creates the new file, renames, HUP's the backends and they reread that file. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck <JanWieck@Yahoo.com> writes:
> If the user changes the config and HUP's the postmaster, postmaster
> rereads the config and merges only those changes, that are changable at
> runtime into it's status. From that status it creates the new file,
> renames, HUP's the backends and they reread that file.
That just moves any potential race conditions to another place, doesn't it?
How's reading this file any safer than reading postgresql.conf? If the
PM gets a second SIGHUP in quick succession, it could be rewriting the
intermediate file while backends are trying to read it.
regards, tom lane
Tom Lane wrote: > Jan Wieck <JanWieck@Yahoo.com> writes: > > If the user changes the config and HUP's the postmaster, postmaster > > rereads the config and merges only those changes, that are changable at > > runtime into it's status. From that status it creates the new file, > > renames, HUP's the backends and they reread that file. > > That just moves any potential race conditions to another place, doesn't it? > How's reading this file any safer than reading postgresql.conf? If the > PM gets a second SIGHUP in quick succession, it could be rewriting the > intermediate file while backends are trying to read it. I have applied the following patch to improve the race condition. With the old code, the nondefault setting file would be written after telling the children to processing the nondefault setting file. Now, the nondefaults file is written before sending the children the SIGHUP. This leaves the only race condition as when a new child is reading the the nondefaults file for the first time. I will make sure WIN32 doesn't lose signals during startup time and processes the new version of the file as well. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/backend/postmaster/postmaster.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/postmaster/postmaster.c,v retrieving revision 1.323 diff -c -c -r1.323 postmaster.c *** src/backend/postmaster/postmaster.c 8 May 2003 14:49:03 -0000 1.323 --- src/backend/postmaster/postmaster.c 8 May 2003 20:30:34 -0000 *************** *** 1582,1592 **** if (Shutdown <= SmartShutdown) { elog(LOG, "Received SIGHUP, reloading configuration files"); - SignalChildren(SIGHUP); ProcessConfigFile(PGC_SIGHUP); #ifdef EXEC_BACKEND write_nondefault_variables(PGC_SIGHUP); #endif load_hba(); load_ident(); } --- 1582,1592 ---- if (Shutdown <= SmartShutdown) { elog(LOG, "Received SIGHUP, reloading configuration files"); ProcessConfigFile(PGC_SIGHUP); #ifdef EXEC_BACKEND write_nondefault_variables(PGC_SIGHUP); #endif + SignalChildren(SIGHUP); load_hba(); load_ident(); }