myProcLocks initialization
I'd like to propose the attached patch, which initializes each
PGPROC's myProcLocks just once at postmaster startup, rather than
every time the PGPROC is handed out to a backend. These lists should
always be emptied before a backend shuts down, so a newly initialized
backend will find the lists empty anyway. Not reinitializing them
shaves a few cycles. In my testing, it saves about 1% of the cost of
setting up and tearing down a connection, which is not a ton, but a
cycle saved is a cycle earned.
Of course, we have a few outstanding reports, like this one from Dave
Gould, indicating that maybe we have a bug in there somewhere:
http://archives.postgresql.org/message-id/20110822073131.GC3363@sonic.net
...but in that case it seems to me that this doesn't make anything
worse than it already is. If the myProcLocks pointers are pointing to
random garbage, we're just kidding ourselves whatever we do; the
system is screwed, and we ought to PANIC, and anything we do here is
laughably inadequate. OTOH, if it just so happens that a backend
found a sneaky way through the exit path that doesn't involve calling
LockReleaseAll(), then overwriting the shm queue pointers removes our
last hope that the next backend can clean up the mess. I'm not
putting a lot of faith in that actually working, just saying that the
current code doesn't seem to be accomplishing anything in the
robustness department.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
init-myproclocks-once.patchapplication/octet-stream; name=init-myproclocks-once.patchDownload
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22cb0b8..d1cceba 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -157,7 +157,8 @@ void
InitProcGlobal(void)
{
PGPROC *procs;
- int i;
+ int i,
+ j;
bool found;
uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS;
@@ -222,6 +223,10 @@ InitProcGlobal(void)
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
ProcGlobal->autovacFreeProcs = &procs[i];
}
+
+ /* Initialize myProcLocks[] shared memory queues. */
+ for (j = 0; j < NUM_LOCK_PARTITIONS; j++)
+ SHMQueueInit(&(procs[i].myProcLocks[j]));
}
/*
@@ -243,7 +248,6 @@ InitProcess(void)
{
/* use volatile pointer to prevent code rearrangement */
volatile PROC_HDR *procglobal = ProcGlobal;
- int i;
/*
* ProcGlobal should be set up already (if we are a backend, we inherit
@@ -303,8 +307,8 @@ InitProcess(void)
MarkPostmasterChildActive();
/*
- * Initialize all fields of MyProc, except for the semaphore and latch,
- * which were prepared for us by InitProcGlobal.
+ * Initialize all fields of MyProc, except for those previously initialized
+ * by InitProcGlobal.
*/
SHMQueueElemInit(&(MyProc->links));
MyProc->waitStatus = STATUS_OK;
@@ -326,8 +330,6 @@ InitProcess(void)
MyProc->lwWaitLink = NULL;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
- for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
- SHMQueueInit(&(MyProc->myProcLocks[i]));
MyProc->recoveryConflictPending = false;
/* Initialize fields for sync rep */
@@ -408,7 +410,6 @@ InitAuxiliaryProcess(void)
{
PGPROC *auxproc;
int proctype;
- int i;
/*
* ProcGlobal should be set up already (if we are a backend, we inherit
@@ -455,8 +456,8 @@ InitAuxiliaryProcess(void)
SpinLockRelease(ProcStructLock);
/*
- * Initialize all fields of MyProc, except for the semaphore and latch,
- * which were prepared for us by InitProcGlobal.
+ * Initialize all fields of MyProc, except for those previously initialized
+ * by InitProcGlobal.
*/
SHMQueueElemInit(&(MyProc->links));
MyProc->waitStatus = STATUS_OK;
@@ -473,8 +474,6 @@ InitAuxiliaryProcess(void)
MyProc->lwWaitLink = NULL;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
- for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
- SHMQueueInit(&(MyProc->myProcLocks[i]));
/*
* Acquire ownership of the PGPROC's latch, so that we can use WaitLatch.
Robert Haas <robertmhaas@gmail.com> writes:
I'd like to propose the attached patch, which initializes each
PGPROC's myProcLocks just once at postmaster startup, rather than
every time the PGPROC is handed out to a backend. These lists should
always be emptied before a backend shuts down, so a newly initialized
backend will find the lists empty anyway. Not reinitializing them
shaves a few cycles. In my testing, it saves about 1% of the cost of
setting up and tearing down a connection, which is not a ton, but a
cycle saved is a cycle earned.
That's not really enough to excite me, and the prospect of problems in
one session corrupting an unrelated later one is pretty scary from a
debugging standpoint. How about at least an Assert that the lock is in
a clean state?
regards, tom lane
On Sun, Oct 30, 2011 at 11:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I'd like to propose the attached patch, which initializes each
PGPROC's myProcLocks just once at postmaster startup, rather than
every time the PGPROC is handed out to a backend. These lists should
always be emptied before a backend shuts down, so a newly initialized
backend will find the lists empty anyway. Not reinitializing them
shaves a few cycles. In my testing, it saves about 1% of the cost of
setting up and tearing down a connection, which is not a ton, but a
cycle saved is a cycle earned.That's not really enough to excite me, and the prospect of problems in
one session corrupting an unrelated later one is pretty scary from a
debugging standpoint. How about at least an Assert that the lock is in
a clean state?
I can go for that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sun, Oct 30, 2011 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Oct 30, 2011 at 11:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I'd like to propose the attached patch, which initializes each
PGPROC's myProcLocks just once at postmaster startup, rather than
every time the PGPROC is handed out to a backend. These lists should
always be emptied before a backend shuts down, so a newly initialized
backend will find the lists empty anyway. Not reinitializing them
shaves a few cycles. In my testing, it saves about 1% of the cost of
setting up and tearing down a connection, which is not a ton, but a
cycle saved is a cycle earned.That's not really enough to excite me, and the prospect of problems in
one session corrupting an unrelated later one is pretty scary from a
debugging standpoint. How about at least an Assert that the lock is in
a clean state?I can go for that.
Revised patch attached. I think it would be useful to assert this
both at process startup time and at process shutdown, since it would
really be much nicer to have the process that didn't clean up fail the
assertion, rather than the new one that innocently inherited its slot;
so the attached patch takes that approach.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
init-myproclocks-once-v2.patchapplication/octet-stream; name=init-myproclocks-once-v2.patchDownload
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22cb0b8..eda3a98 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -157,7 +157,8 @@ void
InitProcGlobal(void)
{
PGPROC *procs;
- int i;
+ int i,
+ j;
bool found;
uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS;
@@ -222,6 +223,10 @@ InitProcGlobal(void)
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->autovacFreeProcs;
ProcGlobal->autovacFreeProcs = &procs[i];
}
+
+ /* Initialize myProcLocks[] shared memory queues. */
+ for (j = 0; j < NUM_LOCK_PARTITIONS; j++)
+ SHMQueueInit(&(procs[i].myProcLocks[j]));
}
/*
@@ -243,7 +248,6 @@ InitProcess(void)
{
/* use volatile pointer to prevent code rearrangement */
volatile PROC_HDR *procglobal = ProcGlobal;
- int i;
/*
* ProcGlobal should be set up already (if we are a backend, we inherit
@@ -303,8 +307,8 @@ InitProcess(void)
MarkPostmasterChildActive();
/*
- * Initialize all fields of MyProc, except for the semaphore and latch,
- * which were prepared for us by InitProcGlobal.
+ * Initialize all fields of MyProc, except for those previously initialized
+ * by InitProcGlobal.
*/
SHMQueueElemInit(&(MyProc->links));
MyProc->waitStatus = STATUS_OK;
@@ -326,8 +330,16 @@ InitProcess(void)
MyProc->lwWaitLink = NULL;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
- for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
- SHMQueueInit(&(MyProc->myProcLocks[i]));
+#ifdef USE_ASSERT_CHECKING
+ if (assert_enabled)
+ {
+ int i;
+
+ /* Last process should have released all locks. */
+ for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
+ Assert(SHMQueueEmpty(&(MyProc->myProcLocks[i])));
+ }
+#endif
MyProc->recoveryConflictPending = false;
/* Initialize fields for sync rep */
@@ -408,7 +420,6 @@ InitAuxiliaryProcess(void)
{
PGPROC *auxproc;
int proctype;
- int i;
/*
* ProcGlobal should be set up already (if we are a backend, we inherit
@@ -455,8 +466,8 @@ InitAuxiliaryProcess(void)
SpinLockRelease(ProcStructLock);
/*
- * Initialize all fields of MyProc, except for the semaphore and latch,
- * which were prepared for us by InitProcGlobal.
+ * Initialize all fields of MyProc, except for those previously initialized
+ * by InitProcGlobal.
*/
SHMQueueElemInit(&(MyProc->links));
MyProc->waitStatus = STATUS_OK;
@@ -473,8 +484,16 @@ InitAuxiliaryProcess(void)
MyProc->lwWaitLink = NULL;
MyProc->waitLock = NULL;
MyProc->waitProcLock = NULL;
- for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
- SHMQueueInit(&(MyProc->myProcLocks[i]));
+#ifdef USE_ASSERT_CHECKING
+ if (assert_enabled)
+ {
+ int i;
+
+ /* Last process should have released all locks. */
+ for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
+ Assert(SHMQueueEmpty(&(MyProc->myProcLocks[i])));
+ }
+#endif
/*
* Acquire ownership of the PGPROC's latch, so that we can use WaitLatch.
@@ -687,6 +706,17 @@ ProcKill(int code, Datum arg)
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
+#ifdef USE_ASSERT_CHECKING
+ if (assert_enabled)
+ {
+ int i;
+
+ /* Last process should have released all locks. */
+ for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
+ Assert(SHMQueueEmpty(&(MyProc->myProcLocks[i])));
+ }
+#endif
+
/*
* Release any LW locks I am holding. There really shouldn't be any, but
* it's cheap to check again before we cut the knees off the LWLock
Robert Haas <robertmhaas@gmail.com> writes:
Revised patch attached. I think it would be useful to assert this
both at process startup time and at process shutdown, since it would
really be much nicer to have the process that didn't clean up fail the
assertion, rather than the new one that innocently inherited its slot;
so the attached patch takes that approach.
+1
regards, tom lane
On Mon, Oct 31, 2011 at 7:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Revised patch attached. I think it would be useful to assert this
both at process startup time and at process shutdown, since it would
really be much nicer to have the process that didn't clean up fail the
assertion, rather than the new one that innocently inherited its slot;
so the attached patch takes that approach.
Something stronger than an assertion at shutdown? Run-time test?
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
On Mon, Oct 31, 2011 at 7:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Revised patch attached. �I think it would be useful to assert this
both at process startup time and at process shutdown, since it would
really be much nicer to have the process that didn't clean up fail the
assertion, rather than the new one that innocently inherited its slot;
so the attached patch takes that approach.
Something stronger than an assertion at shutdown? Run-time test?
There's currently no evidence to suggest this will ever fire at all,
especially not in non-development builds, so an assert seems enough
to me.
regards, tom lane