Reduced power consumption in autovacuum launcher process
On 18 July 2011 01:48, Robert Haas <robertmhaas@gmail.com> wrote:
Might be good to start a new thread for each auxilliary process, or we
may get confused.
Right - I've done so in this reply. There isn't a problem as such with
the AV Launcher patch - there's a problem with most or all remaining
auxiliary processes, that needs to be worked out once and for all. I
refer to the generic signal handler/timeout invalidation issues
already described.
ISTM that these generic handlers ought to be handling this
generically, and that there should be a Latch for just this purpose
for each process within PGPROC. We already have this Latch in PGPROC:Latch waitLatch; /* allow us to wait for sync rep */
Maybe its purpose should be expanded to "current process Latch"?
I think that could be a really good idea, though I haven't looked at
it closely enough to be sure.Another concern is, what happens when we receive a signal, generically
handled or otherwise, and have to SetLatch() to avoid time-out
invalidation? Should we just live with a spurious
AutoVacLauncherMain() iteration, or should we do something like check
if the return value of WaitLatch indicates that we woke up due to a
SetLatch() call, which must have been within a singal handler, and
that we should therefore goto just before WaitLatch() and elide the
spurious iteration? Given that we can expect some signals to occur
relatively frequently, spurious iterations could be a real concern.Really? I suspect that it doesn't much matter exactly how many
machine language instructions we execute on each wake-up, within
reasonable bounds, of course. Maybe some testing is in order?
There's only one way to get around the time-out invalidation problem
that I'm aware of - call SetLatch() in the handler. I'd be happy to
hear alternatives, but until we have an alternative, we're stuck
managing this in each and every signal handler.
Once we've had the latch set to handle this, and control returns to
the auxiliary process loop, we now have to decide from within the
auxiliary if we can figure out that all that happened was a "required"
wake-up, and thus we shouldn't really go through with another
iteration. That, or we can simply do the iteration.
I have my doubts that it is acceptable to wake-up spuriously in
response to routine events that there are generic handlers for. Maybe
this needs to be decided on a case-by-case basis.
On another note, I might be inclined to write something like:
if ((return_value_of_waitlatch & WL_POSTMASTER_DEATH) && !PostmasterIsAlive())
proc_exit(1);...so as to avoid calling that function unnecessarily on every iteration.
Hmm. I'm not so sure. We're now relying on the return value of
WaitLatch(), which isn't guaranteed to report all wake-up events
(although I don't believe it would be a problem in this exact case).
Previously, we called PostmasterIsAlive() once a second anyway, and
that wasn't much of a problem.
Incidentally, should I worry about the timeout long for WaitLatch()
overflowing?How would that happen?
struct timeval is comprised of two longs - one representing seconds,
and the other represented the balance of microseconds. Previously, we
didn't combine them into a single microsecond representation. Now, we
do.
There could perhaps be a very large "nap", as determined by
launcher_determine_sleep(), so that the total number of microseconds
passed to WaitLatch() would exceed the maximum long size that can be
safely represented on some or all platforms. On most 32-bit machines,
sizeof(long) == sizeof(int), which is just 4 bytes. (2^31) - 1 =
2,147,483,647 microseconds = only about 35 minutes. There are corner
cases, such as if someone were to set autovacuum_naptime to something
silly.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
On Jul18, 2011, at 15:12 , Peter Geoghegan wrote:
struct timeval is comprised of two longs - one representing seconds,
and the other represented the balance of microseconds. Previously, we
didn't combine them into a single microsecond representation. Now, we
do.
I haven't actually looked at the code, so maybe I'm missing something
obvious - but why don't we use a double precision float (double) instead
of a long for the combined representation?
best regards,
Florian Pflug
On Mon, Jul 18, 2011 at 9:12 AM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
Another concern is, what happens when we receive a signal, generically
handled or otherwise, and have to SetLatch() to avoid time-out
invalidation? Should we just live with a spurious
AutoVacLauncherMain() iteration, or should we do something like check
if the return value of WaitLatch indicates that we woke up due to a
SetLatch() call, which must have been within a singal handler, and
that we should therefore goto just before WaitLatch() and elide the
spurious iteration? Given that we can expect some signals to occur
relatively frequently, spurious iterations could be a real concern.Really? I suspect that it doesn't much matter exactly how many
machine language instructions we execute on each wake-up, within
reasonable bounds, of course. Maybe some testing is in order?There's only one way to get around the time-out invalidation problem
that I'm aware of - call SetLatch() in the handler. I'd be happy to
hear alternatives, but until we have an alternative, we're stuck
managing this in each and every signal handler.Once we've had the latch set to handle this, and control returns to
the auxiliary process loop, we now have to decide from within the
auxiliary if we can figure out that all that happened was a "required"
wake-up, and thus we shouldn't really go through with another
iteration. That, or we can simply do the iteration.I have my doubts that it is acceptable to wake-up spuriously in
response to routine events that there are generic handlers for. Maybe
this needs to be decided on a case-by-case basis.
I'm confused. If the process gets hit with a signal, it's already
woken up, isn't it? Whatever system call it was blocked on may or may
not get restarted depending on the platform and what the signal
handler does, but from an OS perspective, the process has already been
allocated a time slice and will run until either the time slice is
exhausted or it again blocks.
On another note, I might be inclined to write something like:
if ((return_value_of_waitlatch & WL_POSTMASTER_DEATH) && !PostmasterIsAlive())
proc_exit(1);...so as to avoid calling that function unnecessarily on every iteration.
Hmm. I'm not so sure. We're now relying on the return value of
WaitLatch(), which isn't guaranteed to report all wake-up events
(although I don't believe it would be a problem in this exact case).
Previously, we called PostmasterIsAlive() once a second anyway, and
that wasn't much of a problem.
Ah. OK.
Incidentally, should I worry about the timeout long for WaitLatch()
overflowing?How would that happen?
struct timeval is comprised of two longs - one representing seconds,
and the other represented the balance of microseconds. Previously, we
didn't combine them into a single microsecond representation. Now, we
do.There could perhaps be a very large "nap", as determined by
launcher_determine_sleep(), so that the total number of microseconds
passed to WaitLatch() would exceed the maximum long size that can be
safely represented on some or all platforms. On most 32-bit machines,
sizeof(long) == sizeof(int), which is just 4 bytes. (2^31) - 1 =
2,147,483,647 microseconds = only about 35 minutes. There are corner
cases, such as if someone were to set autovacuum_naptime to something
silly.
OK. In that case, my feeling is "yes, you need to worry about that".
I'm not sure exactly what the best solution is: we could either
twiddle the WaitLatch interface some more, or restrict
autovacuum_naptime to at most 30 minutes, or maybe there's some other
option.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jul 18, 2011 at 9:12 AM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
There could perhaps be a very large "nap", as determined by
launcher_determine_sleep(), so that the total number of microseconds
passed to WaitLatch() would exceed the maximum long size that can be
safely represented on some or all platforms. On most 32-bit machines,
sizeof(long) == sizeof(int), which is just 4 bytes. (2^31) - 1 =
2,147,483,647 microseconds = only about 35 minutes. There are corner
cases, such as if someone were to set autovacuum_naptime to something
silly.
OK. In that case, my feeling is "yes, you need to worry about that".
I'm not sure exactly what the best solution is: we could either
twiddle the WaitLatch interface some more, or restrict
autovacuum_naptime to at most 30 minutes, or maybe there's some other
option.
A wakeup once every half hour would surely not be an issue from a power
consumption standpoint. However, I'm not sure I understand here: aren't
we trying to remove the timeouts completely?
regards, tom lane
On Mon, Jul 18, 2011 at 10:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jul 18, 2011 at 9:12 AM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
There could perhaps be a very large "nap", as determined by
launcher_determine_sleep(), so that the total number of microseconds
passed to WaitLatch() would exceed the maximum long size that can be
safely represented on some or all platforms. On most 32-bit machines,
sizeof(long) == sizeof(int), which is just 4 bytes. (2^31) - 1 =
2,147,483,647 microseconds = only about 35 minutes. There are corner
cases, such as if someone were to set autovacuum_naptime to something
silly.OK. In that case, my feeling is "yes, you need to worry about that".
I'm not sure exactly what the best solution is: we could either
twiddle the WaitLatch interface some more, or restrict
autovacuum_naptime to at most 30 minutes, or maybe there's some other
option.A wakeup once every half hour would surely not be an issue from a power
consumption standpoint. However, I'm not sure I understand here: aren't
we trying to remove the timeouts completely?
Well, in the case of the AV launcher, the issue is that the main loop
is timed by definition, cf. autovacuum_naptime, and the WaitLatch()
interface is apparently designed so that we can't sleep longer than 35
minutes. We can avoid waking every second simply to check whether the
postmaster has died, but waking up every autovacuum_naptime seems de
rigeur barring a major redesign of the mechanism.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jul 18, 2011 at 10:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
A wakeup once every half hour would surely not be an issue from a power
consumption standpoint. �However, I'm not sure I understand here: aren't
we trying to remove the timeouts completely?
Well, in the case of the AV launcher, the issue is that the main loop
is timed by definition, cf. autovacuum_naptime, and the WaitLatch()
interface is apparently designed so that we can't sleep longer than 35
minutes.
Hmm. Well, it's not too late to rethink the WaitLatch API, if we think
that that might be a significant limitation. Offhand I don't see that
it is, though.
regards, tom lane
On 18.07.2011 18:32, Tom Lane wrote:
Robert Haas<robertmhaas@gmail.com> writes:
On Mon, Jul 18, 2011 at 10:35 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
A wakeup once every half hour would surely not be an issue from a power
consumption standpoint. However, I'm not sure I understand here: aren't
we trying to remove the timeouts completely?Well, in the case of the AV launcher, the issue is that the main loop
is timed by definition, cf. autovacuum_naptime, and the WaitLatch()
interface is apparently designed so that we can't sleep longer than 35
minutes.Hmm. Well, it's not too late to rethink the WaitLatch API, if we think
that that might be a significant limitation.
Right, we can easily change the timeout argument to be in milliseconds
instead of microseconds.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
On 18.07.2011 18:32, Tom Lane wrote:
Hmm. Well, it's not too late to rethink the WaitLatch API, if we think
that that might be a significant limitation.
Right, we can easily change the timeout argument to be in milliseconds
instead of microseconds.
On the whole I'd be more worried about giving up the shorter waits than
the longer ones --- it's not too hard to imagine using submillisecond
timeouts in the future, as machines get faster. If we really wanted to
fix this, I think we need to move to a wider datatype.
regards, tom lane
On Mon, Jul 18, 2011 at 03:12:20PM -0400, Tom Lane wrote:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
On 18.07.2011 18:32, Tom Lane wrote:
Hmm. Well, it's not too late to rethink the WaitLatch API, if we think
that that might be a significant limitation.Right, we can easily change the timeout argument to be in milliseconds
instead of microseconds.On the whole I'd be more worried about giving up the shorter waits than
the longer ones --- it's not too hard to imagine using submillisecond
timeouts in the future, as machines get faster. If we really wanted to
fix this, I think we need to move to a wider datatype.regards, tom lane
You could also tag the high bit to allow you to encode larger ranges
by having microseconds for the values with the high bit = 0 and use
milliseconds for the values with the high bit = 1. Then you could
have the best of both without punching up the width of the datatype.
Regard,
Ken
On Mon, Jul 18, 2011 at 3:28 PM, ktm@rice.edu <ktm@rice.edu> wrote:
On Mon, Jul 18, 2011 at 03:12:20PM -0400, Tom Lane wrote:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
On 18.07.2011 18:32, Tom Lane wrote:
Hmm. Well, it's not too late to rethink the WaitLatch API, if we think
that that might be a significant limitation.Right, we can easily change the timeout argument to be in milliseconds
instead of microseconds.On the whole I'd be more worried about giving up the shorter waits than
the longer ones --- it's not too hard to imagine using submillisecond
timeouts in the future, as machines get faster. If we really wanted to
fix this, I think we need to move to a wider datatype.regards, tom lane
You could also tag the high bit to allow you to encode larger ranges
by having microseconds for the values with the high bit = 0 and use
milliseconds for the values with the high bit = 1. Then you could
have the best of both without punching up the width of the datatype.
Considering that we're just talking about a function call here, and
not something that ever goes out to disk, that seems like entirely too
much notational complexity.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 18 July 2011 20:06, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Hmm. Well, it's not too late to rethink the WaitLatch API, if we think
that that might be a significant limitation.Right, we can easily change the timeout argument to be in milliseconds
instead of microseconds.
+1
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
Attached is revision of this patch that now treats the latch in
PGPROC, waitLatch, as the generic "process latch", rather than just
using it for sync rep; It is initialised appropriately as a shared
latch generically, within InitProcGlobal(), and ownership is
subsequently set within InitProcess(). We were doing so before, though
only for the benefit of sync rep.
The idea here is to have a per-process latch to guard against time-out
invalidation issues from within each generic signal handler, by
calling SetLatch() on our generic latch there, much as we already do
from within non-generic archiver process signal handlers on the
archiver's static, non-generic latch (the archiver has its MyProc
pointer set to NULL, and we allow for the possibility that generic
handlers may be registered within processes that have a NULL MyProc -
though latch timeout invalidation issues then become the
responsibility of the process exclusively, just as is currently the
case with the archiver. In other words, they better not register a
generic signal handler).
It doesn't really matter that the SetLatch() call will usually be
unnecessary, because, as Heikki once pointed out, redundantly setting
a latch that is already set is very cheap. We don't check if it's set
directly in advance of setting the latch, because the Latch struct is
"logically opaque" and there is no "public function" to check if it's
set, nor should there be; the checking simply happens in SetLatch().
On 18 July 2011 20:06, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Right, we can easily change the timeout argument to be in milliseconds
instead of microseconds.
I've done so in this latest revision as a precautionary measure. I
don't see much point in sub-millisecond granularity, and besides, the
Windows implementation will not provide that granularity anyway as
things stand.
Thoughts?
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
Attachments:
avlauncher_latch.v2.patchtext/x-patch; charset=US-ASCII; name=avlauncher_latch.v2.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6a6959f..b2cf973 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10210,7 +10210,7 @@ retry:
/*
* Wait for more WAL to arrive, or timeout to be reached
*/
- WaitLatch(&XLogCtl->recoveryWakeupLatch, WL_LATCH_SET | WL_TIMEOUT, 5000000L);
+ WaitLatch(&XLogCtl->recoveryWakeupLatch, WL_LATCH_SET | WL_TIMEOUT, 5000L);
ResetLatch(&XLogCtl->recoveryWakeupLatch);
}
else
diff --git a/src/backend/port/unix_latch.c b/src/backend/port/unix_latch.c
index 9940a42..0663eb8 100644
--- a/src/backend/port/unix_latch.c
+++ b/src/backend/port/unix_latch.c
@@ -182,7 +182,7 @@ DisownLatch(volatile Latch *latch)
* to wait for. If the latch is already set (and WL_LATCH_SET is given), the
* function returns immediately.
*
- * The 'timeout' is given in microseconds. It must be >= 0 if WL_TIMEOUT
+ * The 'timeout' is given in milliseconds. It must be >= 0 if WL_TIMEOUT
* event is given, otherwise it is ignored. On some platforms, signals cause
* the timeout to be restarted, so beware that the function can sleep for
* several times longer than the specified timeout.
@@ -236,8 +236,9 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
if (wakeEvents & WL_TIMEOUT)
{
Assert(timeout >= 0);
- tv.tv_sec = timeout / 1000000L;
- tv.tv_usec = timeout % 1000000L;
+ tv.tv_sec = timeout / 1000L;
+ /* tv_usec should be in microseconds */
+ tv.tv_usec = (timeout % 1000L) * 1000L;
tvp = &tv;
}
@@ -329,6 +330,11 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
/*
* Sets a latch and wakes up anyone waiting on it. Returns quickly if the
* latch is already set.
+ *
+ * Note that there is a general requirement to call this function within
+ * signal handlers, to ensure that latch timeout is not invalidated. For
+ * generic signal handlers that may be registered for multiple processes,
+ * it will be called with the per-process latch in PGPROC as its argument.
*/
void
SetLatch(volatile Latch *latch)
diff --git a/src/backend/port/win32_latch.c b/src/backend/port/win32_latch.c
index 8d59ad4..5ee4f40 100644
--- a/src/backend/port/win32_latch.c
+++ b/src/backend/port/win32_latch.c
@@ -99,23 +99,14 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, SOCKET sock,
int numevents;
int result = 0;
int pmdeath_eventno = 0;
- long timeout_ms;
Assert(wakeEvents != 0);
+ Assert(timeout >= 0 || (wakeEvents & WL_TIMEOUT) == 0);
/* Ignore WL_SOCKET_* events if no valid socket is given */
if (sock == PGINVALID_SOCKET)
wakeEvents &= ~(WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE);
- /* Convert timeout to milliseconds for WaitForMultipleObjects() */
- if (wakeEvents & WL_TIMEOUT)
- {
- Assert(timeout >= 0);
- timeout_ms = timeout / 1000;
- }
- else
- timeout_ms = INFINITE;
-
/* Construct an array of event handles for WaitforMultipleObjects() */
latchevent = latch->event;
@@ -162,7 +153,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, SOCKET sock,
break;
}
- rc = WaitForMultipleObjects(numevents, events, FALSE, timeout_ms);
+ rc = WaitForMultipleObjects(numevents, events, FALSE, (wakeEvents & WL_TIMEOUT)?timeout:INFINITE);
if (rc == WAIT_FAILED)
elog(ERROR, "WaitForMultipleObjects() failed: error code %d", (int) GetLastError());
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2f3fcbf..acfc3b4 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -84,6 +84,7 @@
#include "postmaster/postmaster.h"
#include "storage/bufmgr.h"
#include "storage/ipc.h"
+#include "storage/latch.h"
#include "storage/pmsignal.h"
#include "storage/proc.h"
#include "storage/procsignal.h"
@@ -567,38 +568,20 @@ AutoVacLauncherMain(int argc, char *argv[])
/*
* Sleep for a while according to schedule.
+ * Wait on Latch.
*
- * On some platforms, signals won't interrupt the sleep. To ensure we
- * respond reasonably promptly when someone signals us, break down the
- * sleep into 1-second increments, and check for interrupts after each
- * nap.
+ * We handle wait time invalidation by calling
+ * SetLatch() in signal handlers.
*/
- while (nap.tv_sec > 0 || nap.tv_usec > 0)
- {
- uint32 sleeptime;
-
- if (nap.tv_sec > 0)
- {
- sleeptime = 1000000;
- nap.tv_sec--;
- }
- else
- {
- sleeptime = nap.tv_usec;
- nap.tv_usec = 0;
- }
- pg_usleep(sleeptime);
-
- /*
- * Emergency bailout if postmaster has died. This is to avoid the
- * necessity for manual cleanup of all postmaster children.
- */
- if (!PostmasterIsAlive())
- proc_exit(1);
-
- if (got_SIGTERM || got_SIGHUP || got_SIGUSR2)
- break;
- }
+ WaitLatch(&MyProc->waitLatch, WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH, (nap.tv_sec * 1000L) + (nap.tv_usec / 1000L));
+ /*
+ * Emergency bailout if postmaster has died. This is to avoid the
+ * necessity for manual cleanup of all postmaster children.
+ *
+ * This happens again here because we may have slept for quite a while.
+ */
+ if (!PostmasterIsAlive())
+ proc_exit(1);
DisableCatchupInterrupt();
@@ -1322,6 +1305,8 @@ static void
avl_sighup_handler(SIGNAL_ARGS)
{
got_SIGHUP = true;
+ /* let the waiting loop iterate */
+ SetLatch(&MyProc->waitLatch);
}
/* SIGUSR2: a worker is up and running, or just finished, or failed to fork */
@@ -1329,6 +1314,8 @@ static void
avl_sigusr2_handler(SIGNAL_ARGS)
{
got_SIGUSR2 = true;
+ /* let the waiting loop iterate */
+ SetLatch(&MyProc->waitLatch);
}
/* SIGTERM: time to die */
@@ -1336,6 +1323,8 @@ static void
avl_sigterm_handler(SIGNAL_ARGS)
{
got_SIGTERM = true;
+ /* let the waiting loop iterate */
+ SetLatch(&MyProc->waitLatch);
}
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 2070fbb..8d1dab7 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -409,7 +409,7 @@ pgarch_MainLoop(void)
int rc;
rc = WaitLatch(&mainloop_latch,
WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH,
- timeout * 1000000L);
+ timeout * 1000L);
if (rc & WL_TIMEOUT)
wakened = true;
}
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 32db2bc..4672a52 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -171,7 +171,7 @@ SyncRepWaitForLSN(XLogRecPtr XactCommitLSN)
* postmaster death regularly while waiting. Note that timeout here
* does not necessarily release from loop.
*/
- WaitLatch(&MyProc->waitLatch, WL_LATCH_SET | WL_TIMEOUT, 60000000L);
+ WaitLatch(&MyProc->waitLatch, WL_LATCH_SET | WL_TIMEOUT, 60000L);
/* Must reset the latch before testing state. */
ResetLatch(&MyProc->waitLatch);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 63a6304..c1996ca 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -812,7 +812,7 @@ WalSndLoop(void)
if (pq_is_send_pending())
wakeEvents |= WL_SOCKET_WRITEABLE;
WaitLatchOrSocket(&MyWalSnd->latch, wakeEvents,
- MyProcPort->sock, sleeptime * 1000L);
+ MyProcPort->sock, sleeptime);
/* Check for replication timeout */
if (replication_timeout > 0 &&
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 28bcaa7..892941c 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -22,6 +22,7 @@
#include "miscadmin.h"
#include "storage/ipc.h"
#include "storage/latch.h"
+#include "storage/proc.h"
#include "storage/procsignal.h"
#include "storage/shmem.h"
#include "storage/sinval.h"
@@ -255,6 +256,9 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
{
int save_errno = errno;
+ if (MyProc)
+ SetLatch(&MyProc->waitLatch);
+
if (CheckProcSignal(PROCSIG_CATCHUP_INTERRUPT))
HandleCatchupInterrupt();
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index f9b3028..8360bd5 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1613,6 +1613,8 @@ handle_sig_alarm(SIGNAL_ARGS)
(void) CheckStatementTimeout();
errno = save_errno;
+ if (MyProc)
+ SetLatch(&MyProc->waitLatch);
}
/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index f035a48..ec91f18 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2643,12 +2643,12 @@ die(SIGNAL_ARGS)
InterruptHoldoffCount--;
ProcessInterrupts();
}
-
- /* Interrupt any sync rep wait which is currently in progress. */
- SetLatch(&(MyProc->waitLatch));
}
errno = save_errno;
+ /* set the process latch - we cannot risk invalidating its timeout */
+ if (MyProc)
+ SetLatch(&MyProc->waitLatch);
}
/*
@@ -2684,18 +2684,22 @@ StatementCancelHandler(SIGNAL_ARGS)
InterruptHoldoffCount--;
ProcessInterrupts();
}
-
- /* Interrupt any sync rep wait which is currently in progress. */
- SetLatch(&(MyProc->waitLatch));
}
errno = save_errno;
+ /* set the process latch - we cannot risk invalidating its timeout */
+ if (MyProc)
+ SetLatch(&MyProc->waitLatch);
}
/* signal handler for floating point exception */
void
FloatExceptionHandler(SIGNAL_ARGS)
{
+ /* set the process latch - we cannot risk invalidating its timeout */
+ if (MyProc)
+ SetLatch(&MyProc->waitLatch);
+
ereport(ERROR,
(errcode(ERRCODE_FLOATING_POINT_EXCEPTION),
errmsg("floating-point exception"),
@@ -2709,6 +2713,9 @@ static void
SigHupHandler(SIGNAL_ARGS)
{
got_SIGHUP = true;
+ /* set the process latch - we cannot risk invalidating its timeout */
+ if (MyProc)
+ SetLatch(&MyProc->waitLatch);
}
/*
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 09ac3cf..71e41d3 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -126,13 +126,14 @@ struct PGPROC
LOCKMASK heldLocks; /* bitmask for lock types already held on this
* lock object by this backend */
+ Latch waitLatch; /* generic latch for process */
+
/*
* Info to allow us to wait for synchronous replication, if needed.
* waitLSN is InvalidXLogRecPtr if not waiting; set only by user backend.
* syncRepState must not be touched except by owning process or WALSender.
* syncRepLinks used only while holding SyncRepLock.
*/
- Latch waitLatch; /* allow us to wait for sync rep */
XLogRecPtr waitLSN; /* waiting for this LSN or higher */
int syncRepState; /* wait state for sync rep */
SHM_QUEUE syncRepLinks; /* list link if process is in syncrep queue */
Peter Geoghegan <peter@2ndquadrant.com> writes:
Attached is revision of this patch that now treats the latch in
PGPROC, waitLatch, as the generic "process latch", rather than just
using it for sync rep; It is initialised appropriately as a shared
latch generically, within InitProcGlobal(), and ownership is
subsequently set within InitProcess(). We were doing so before, though
only for the benefit of sync rep.
Now that I've got the WaitLatch code fully swapped into my head,
I'm thinking of pushing on to review/commit this patch of Peter's.
On 18 July 2011 20:06, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:Right, we can easily change the timeout argument to be in milliseconds
instead of microseconds.
I've done so in this latest revision as a precautionary measure. I
don't see much point in sub-millisecond granularity, and besides, the
Windows implementation will not provide that granularity anyway as
things stand.
I did not see any objections to such a change. I think we should pull
out this aspect and commit it to 9.1 as well as HEAD. That will provide
one less gotcha for anyone who develops against the 9.1 latch code and
later needs to port to 9.2.
regards, tom lane
On 9 August 2011 23:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Now that I've got the WaitLatch code fully swapped into my head,
I'm thinking of pushing on to review/commit this patch of Peter's.
Thanks for giving this your attention. I had already planned to
produce a new revision this weekend, so I'd appreciate it if you could
hold off until Sunday or Monday.
I did not see any objections to such a change. I think we should pull
out this aspect and commit it to 9.1 as well as HEAD. That will provide
one less gotcha for anyone who develops against the 9.1 latch code and
later needs to port to 9.2.
That is a good point. I'm aware that someone already made the mistake
of giving the value of timeout as milliseconds rather than
microseconds at one point, so this seems to be a fertile source of
confusion.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
Peter Geoghegan <peter@2ndquadrant.com> writes:
On 9 August 2011 23:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Now that I've got the WaitLatch code fully swapped into my head,
I'm thinking of pushing on to review/commit this patch of Peter's.
Thanks for giving this your attention. I had already planned to
produce a new revision this weekend, so I'd appreciate it if you could
hold off until Sunday or Monday.
Actually, I'm nearly done with it already. Perhaps you could start
thinking about the other polling loops.
regards, tom lane
On 10 August 2011 01:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, I'm nearly done with it already. Perhaps you could start
thinking about the other polling loops.
Fair enough. I'm slightly surprised that there doesn't need to be some
bikeshedding about my idea to treat the PGPROC latch as the generic,
per-process latch.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
Peter Geoghegan <peter@2ndquadrant.com> writes:
On 10 August 2011 01:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, I'm nearly done with it already. �Perhaps you could start
thinking about the other polling loops.
Fair enough. I'm slightly surprised that there doesn't need to be some
bikeshedding about my idea to treat the PGPROC latch as the generic,
per-process latch.
No, I don't find that unreasonable, especially not since Simon had made
that the de facto situation anyhow by having it be initialized for all
backends in proc.c and set unconditionally by some of the standard
signal handlers. I am working on renaming it to procLatch (I find
"waitLatch" a bit too generic) and fixing a bunch of pre-existing bugs
that I now see in that code, like failure to save/restore errno in
signal handlers that used to only set a flag but now also call SetLatch.
regards, tom lane
Peter Geoghegan <peter@2ndquadrant.com> writes:
Attached is revision of this patch that now treats the latch in
PGPROC, waitLatch, as the generic "process latch", rather than just
using it for sync rep; It is initialised appropriately as a shared
latch generically, within InitProcGlobal(), and ownership is
subsequently set within InitProcess(). We were doing so before, though
only for the benefit of sync rep.
Applied with some corrections, notably:
* Signal handlers mustn't change errno and mustn't assume MyProc is set,
since they might get invoked before it's set or after it's cleared.
Calling SetLatch outside the save/restore of errno is right out, of
course, but I also found that quite a lot of handlers had previously
been hacked to call SetLatch or latch_sigusr1_handler without any
save/restore at all. I'm not sure if any of those were your mistake;
I think a lot of them were pre-existing bugs in the sync rep patch.
* avlauncher loop was missing a ResetLatch call. I also added a comment
explaining the difference from the normal pattern for using WaitLatch.
* I didn't add a SetLatch call in procsignal_sigusr1_handler. I'm
unconvinced that it's necessary, and if it is we probably need to
rethink using SIGUSR1 internally in unix_latch.c. It does not make
sense to set the procLatch when we get a signal that's directed towards
releasing some other latch.
regards, tom lane
On Wed, Aug 10, 2011 at 3:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Geoghegan <peter@2ndquadrant.com> writes:
On 10 August 2011 01:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, I'm nearly done with it already. Perhaps you could start
thinking about the other polling loops.Fair enough. I'm slightly surprised that there doesn't need to be some
bikeshedding about my idea to treat the PGPROC latch as the generic,
per-process latch.No, I don't find that unreasonable, especially not since Simon had made
that the de facto situation anyhow by having it be initialized for all
backends in proc.c and set unconditionally by some of the standard
signal handlers. I am working on renaming it to procLatch (I find
"waitLatch" a bit too generic) and
That was the direction I wanted to go in anyway, as you guessed.
fixing a bunch of pre-existing bugs
that I now see in that code, like failure to save/restore errno in
signal handlers that used to only set a flag but now also call SetLatch.
Thanks for looking at the code; sounds like we nipped a few
would-have-been-bugs there.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services