Обсуждение: pg_postmaster_reload_time() patch
I've done a quick write up for reload time reporting from the administration TODO. I was a little paranoid with the locking, but didn't want problems to occur with signals on the postmaster and the read side. -George
Вложения
On Wed, Apr 30, 2008 at 9:53 AM, George Gensure <werkt0@gmail.com> wrote:
IMHO, the function should return NULL if the postmaster never reloaded; that is, it was started, but never signaled to reload.
Best regards,
--
gurjeet[.singh]@EnterpriseDB.com
singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
EnterpriseDB http://www.enterprisedb.com
Mail sent from my BlackLaptop device
I've done a quick write up for reload time reporting from the
administration TODO. I was a little paranoid with the locking, but
didn't want problems to occur with signals on the postmaster and the
read side.
IMHO, the function should return NULL if the postmaster never reloaded; that is, it was started, but never signaled to reload.
Best regards,
--
gurjeet[.singh]@EnterpriseDB.com
singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
EnterpriseDB http://www.enterprisedb.com
Mail sent from my BlackLaptop device
Gurjeet Singh wrote: > On Wed, Apr 30, 2008 at 9:53 AM, George Gensure <werkt0@gmail.com> wrote: > >> I've done a quick write up for reload time reporting from the >> administration TODO. I was a little paranoid with the locking, but >> didn't want problems to occur with signals on the postmaster and the >> read side. >> >> > IMHO, the function should return NULL if the postmaster never reloaded; that > is, it was started, but never signaled to reload. I think it's useful to get the server startup time if the configuration has never been reloaded. That's when the configuration was loaded, if no reload has been triggered since. Perhaps the function should be named to reflect that better. pg_configuration_load_time() ? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
George Gensure escribió: > I've done a quick write up for reload time reporting from the > administration TODO. I was a little paranoid with the locking, but > didn't want problems to occur with signals on the postmaster and the > read side. I'd say too much -- postmaster runs with signals blocked all the time (except during select()) so this is not necessary there. Regarding the locking on backends, I admit I am not sure if this is really a problem enough that you need a spinlock for it. Anyway we tend not to use spinlocks too much -- probably an LWLock would be more apropos, if a lock is really needed. (A bigger question is whether the reload time should be local for each backend, or exposed globally through MyProc. I don't think it's interesting enough to warrant that, but perhaps others think differently.) Lastly, I didn't read the patch close enough to tell if it would work on both the EXEC_BACKEND case and the regular one. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Wed, Apr 30, 2008 at 8:16 AM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > George Gensure escribió: > > > > I've done a quick write up for reload time reporting from the > > administration TODO. I was a little paranoid with the locking, but > > didn't want problems to occur with signals on the postmaster and the > > read side. > > I'd say too much -- postmaster runs with signals blocked all the time > (except during select()) so this is not necessary there. > > Regarding the locking on backends, I admit I am not sure if this is > really a problem enough that you need a spinlock for it. Anyway we tend > not to use spinlocks too much -- probably an LWLock would be more > apropos, if a lock is really needed. (A bigger question is whether the > reload time should be local for each backend, or exposed globally > through MyProc. I don't think it's interesting enough to warrant that, > but perhaps others think differently.) > > Lastly, I didn't read the patch close enough to tell if it would work on > both the EXEC_BACKEND case and the regular one. > > -- > Alvaro Herrera http://www.CommandPrompt.com/ > The PostgreSQL Company - Command Prompt, Inc. > I've reworked the patch in response to comments. The new function name is pg_conf_load_time() I'm now using LWLocks only on the backend in order to protect the PgReloadTime from mid copy reads. This may prove to be unnecessary, since the code to handle HUPs seems to be executed synchronously on the backend, but I'll let someone else tell me its safe before removing it. -George
Вложения
On Wed, Apr 30, 2008 at 12:58 PM, George Gensure <werkt0@gmail.com> wrote: > On Wed, Apr 30, 2008 at 8:16 AM, Alvaro Herrera > <alvherre@commandprompt.com> wrote: > > > > George Gensure escribió: > > > > > > > I've done a quick write up for reload time reporting from the > > > administration TODO. I was a little paranoid with the locking, but > > > didn't want problems to occur with signals on the postmaster and the > > > read side. > > > > I'd say too much -- postmaster runs with signals blocked all the time > > (except during select()) so this is not necessary there. > > > > Regarding the locking on backends, I admit I am not sure if this is > > really a problem enough that you need a spinlock for it. Anyway we tend > > not to use spinlocks too much -- probably an LWLock would be more > > apropos, if a lock is really needed. (A bigger question is whether the > > reload time should be local for each backend, or exposed globally > > through MyProc. I don't think it's interesting enough to warrant that, > > but perhaps others think differently.) > > > > Lastly, I didn't read the patch close enough to tell if it would work on > > both the EXEC_BACKEND case and the regular one. > > > > -- > > Alvaro Herrera http://www.CommandPrompt.com/ > > The PostgreSQL Company - Command Prompt, Inc. > > > > I've reworked the patch in response to comments. > > The new function name is pg_conf_load_time() > I'm now using LWLocks only on the backend in order to protect the > PgReloadTime from mid copy reads. This may prove to be unnecessary, > since the code to handle HUPs seems to be executed synchronously on > the backend, but I'll let someone else tell me its safe before > removing it. > > -George > So if nobody's got any further objections, could this patch be applied? -George
George Gensure escribió: > So if nobody's got any further objections, could this patch be applied? It's in the queue: http://wiki.postgresql.org/wiki/CommitFest:May -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Fri, May 2, 2008 at 10:31 AM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > George Gensure escribió: > > > > So if nobody's got any further objections, could this patch be applied? > > It's in the queue: > > http://wiki.postgresql.org/wiki/CommitFest:May > > -- > > > Alvaro Herrera http://www.CommandPrompt.com/ > The PostgreSQL Company - Command Prompt, Inc. > Thanks! -George
"George Gensure" <werkt0@gmail.com> writes: > The new function name is pg_conf_load_time() Applied with revisions. > I'm now using LWLocks only on the backend in order to protect the > PgReloadTime from mid copy reads. This may prove to be unnecessary, > since the code to handle HUPs seems to be executed synchronously on > the backend, but I'll let someone else tell me its safe before > removing it. The locking was not only entirely useless, but it didn't even compile, since you'd not supplied a definition for "ReloadTimeLock". I took it out. I moved the setting of PgReloadTime into ProcessConfigFile. The advantages are (1) only one place to do it, and (2) inside ProcessConfigFile, we know whether or not the reload actually "took". As committed, the definition of PgReloadTime is really "the time of the last *successful* load of postgresql.conf", which I think is more useful than "the last attempted load". I also improved the documentation a bit and added the copy step needed in restore_backend_variables(). regards, tom lane