Re: [HACKERS] patch: fix SSI finished list corruption

2012-01-18 Thread Heikki Linnakangas

On 07.01.2012 02:15, Dan Ports wrote:

There's a corner case in the SSI cleanup code that isn't handled
correctly. It can arise when running workloads that are comprised
mostly (but not 100%) of READ ONLY transactions, and can corrupt the
finished SERIALIZABLEXACT list, potentially causing a segfault. The
attached patch fixes it.

Specifically, when the only remaining active transactions are READ
ONLY, we do a "partial cleanup" of committed transactions because
certain types of conflicts aren't possible anymore. For committed r/w
transactions, we release the SIREAD locks but keep the
SERIALIZABLEXACT. However, for committed r/o transactions, we can go
further and release the SERIALIZABLEXACT too. The problem was with the
latter case: we were returning the SERIALIZABLEXACT to the free list
without removing it from the finished list.

The only real change in the patch is the SHMQueueDelete line, but I
also reworked some of the surrounding code to make it obvious that r/o
and r/w transactions are handled differently -- the existing code felt
a bit too clever.


Thanks, committed!

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] patch: fix SSI finished list corruption

2012-01-06 Thread Dan Ports
There's a corner case in the SSI cleanup code that isn't handled
correctly. It can arise when running workloads that are comprised
mostly (but not 100%) of READ ONLY transactions, and can corrupt the
finished SERIALIZABLEXACT list, potentially causing a segfault. The
attached patch fixes it.

Specifically, when the only remaining active transactions are READ
ONLY, we do a "partial cleanup" of committed transactions because
certain types of conflicts aren't possible anymore. For committed r/w
transactions, we release the SIREAD locks but keep the
SERIALIZABLEXACT. However, for committed r/o transactions, we can go
further and release the SERIALIZABLEXACT too. The problem was with the
latter case: we were returning the SERIALIZABLEXACT to the free list
without removing it from the finished list.

The only real change in the patch is the SHMQueueDelete line, but I
also reworked some of the surrounding code to make it obvious that r/o
and r/w transactions are handled differently -- the existing code felt
a bit too clever.

Dan

-- 
Dan R. K. Ports  MIT CSAILhttp://drkp.net/
>From 39f8462332f998d7363058adabac412c7654befe Mon Sep 17 00:00:00 2001
From: "Dan R. K. Ports" 
Date: Thu, 29 Dec 2011 23:11:35 -0500
Subject: [PATCH] Read-only SERIALIZABLEXACTs are completely released when
 doing partial cleanup, so remove them from the finished
 list. This prevents the finished list from being corrupted.
 Also make it more clear that read-only transactions are
 treated differently here.

---
 src/backend/storage/lmgr/predicate.c |   26 +++---
 1 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index aefa698..c0b3cb4 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -3531,10 +3531,29 @@ ClearOldPredicateLocks(void)
 		else if (finishedSxact->commitSeqNo > PredXact->HavePartialClearedThrough
 		   && finishedSxact->commitSeqNo <= PredXact->CanPartialClearThrough)
 		{
+			/*
+			 * Any active transactions that took their snapshot before
+			 * this transaction committed are read-only, so we can
+			 * clear part of its state.
+			 */
 			LWLockRelease(SerializableXactHashLock);
-			ReleaseOneSerializableXact(finishedSxact,
-	   !SxactIsReadOnly(finishedSxact),
-	   false);
+
+			if (SxactIsReadOnly(finishedSxact))
+			{
+/* A read-only transaction can be removed entirely */
+SHMQueueDelete(&(finishedSxact->finishedLink));
+ReleaseOneSerializableXact(finishedSxact, false, false);
+			}
+			else
+			{
+/*
+ * A read-write transaction can only be partially
+ * cleared. We need to keep the SERIALIZABLEXACT but
+ * can release the SIREAD locks and conflicts in.
+ */
+ReleaseOneSerializableXact(finishedSxact, true, false);
+			}
+			
 			PredXact->HavePartialClearedThrough = finishedSxact->commitSeqNo;
 			LWLockAcquire(SerializableXactHashLock, LW_SHARED);
 		}
@@ -3640,6 +3659,7 @@ ReleaseOneSerializableXact(SERIALIZABLEXACT *sxact, bool partial,
 
 	Assert(sxact != NULL);
 	Assert(SxactIsRolledBack(sxact) || SxactIsCommitted(sxact));
+	Assert(partial || !SxactIsOnFinishedList(sxact));
 	Assert(LWLockHeldByMe(SerializableFinishedListLock));
 
 	/*
-- 
1.7.8.2


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers