Message-ID: <4E160005.1020300@enterprisedb.com>
Date: Thu, 07 Jul 2011 21:50:45 +0300
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; fi-FI;
	rv:1.9.2.17) Gecko/20110606 Icedove/3.1.10
MIME-Version: 1.0
To: Kevin Grittner <Kevin.Grittner@wicourts.gov>
CC: pgsql-hackers@postgresql.org
Subject: Re: SSI atomic commit
References: <4E12FDA8020000250003EFC1@gw.wicourts.gov>
	<4E15D48A.7080203@enterprisedb.com>
	<4E159B5C020000250003F063@gw.wicourts.gov>
In-Reply-To: <4E159B5C020000250003F063@gw.wicourts.gov>
Content-Type: multipart/mixed; boundary="------------040703000502070501010607"

This is a multi-part message in MIME format.
--------------040703000502070501010607
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 07.07.2011 19:41, Kevin Grittner wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  wrote:
>> On 05.07.2011 20:03, Kevin Grittner wrote:
>>> In reviewing the 2PC changes mentioned in a separate post, both
>>> Dan and I realized that these were dependent on the assumption
>>> that SSI's commitSeqNo is assigned in the order in which the
>>> transactions became visible.
>>
>> This comment in the patch actually suggests a stronger
>> requirement:
>>
>>> + * For correct SERIALIZABLE semantics, the commitSeqNo must
>>>      appear to be set
>>> + * atomically with the work of the transaction becoming visible
>>>      to other
>>> + * transactions.
>>
>> So, is it enough for the commitSeqNos to be set in the order the
>> transactions become visible to others? I'm assuming 'yes' for now,
>> as the approach being discussed to assign commitSeqNo in
>> ProcArrayEndTransaction() without also holding
>> SerializableXactHashLock is not going to work otherwise, and if I
>> understood correctly you didn't see any correctness issue with
>> that. Please shout if I'm missing something.
>
> Hmm.  The more strict semantics are much easier to reason about and
> ensure correctness.

True.

>  I think that the looser semantics can be made
> to work, but there needs to be more fussy logic in the SSI code to
> deal with the fact that we don't know whether the visibility change
> has occurred.  Really what pushed us to the patch using the stricter
> semantics was how much the discussion of how to cover the edge
> conditions with the looser semantics made our heads spin.  Anything
> that confusing seems more prone to subtle bugs.

Let's step back and analyse a bit closer what goes wrong with the 
current semantics. The problem is that commitSeqNo is assigned too late, 
sometime after the transaction has become visible to others.

Looking at the comparisons that we do with commitSeqNos, they all have 
conservative direction that we can err to, when in doubt. So here's an idea:

Let's have two sequence numbers for each transaction: prepareSeqNo and 
commitSeqNo. prepareSeqNo is assigned when a transaction is prepared (in 
PreCommit_CheckForSerializableConflicts), and commitSeqNo is assigned 
when it's committed (in ReleasePredicateLocks). They are both assigned 
from one counter, LastSxactCommitSeqNo, so that is advanced twice per 
transaction, and prepareSeqNo is always smaller than commitSeqNo for a 
transaction. Modify operations that currently use commitSeqNo to use 
either prepareSeqNo or commitSeqNo, so that we err on the safe side.

That yields a much smaller patch (attached). How does this look to you, 
am I missing anything?

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

--------------040703000502070501010607
Content-Type: text/x-diff;
 name="track-prepareseqno-1.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="track-prepareseqno-1.patch"

diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index f0c8ee4..134a0a0 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -1184,6 +1184,7 @@ InitPredicateLocks(void)
 		}
 		PredXact->OldCommittedSxact = CreatePredXact();
 		SetInvalidVirtualTransactionId(PredXact->OldCommittedSxact->vxid);
+		PredXact->OldCommittedSxact->prepareSeqNo = 0;
 		PredXact->OldCommittedSxact->commitSeqNo = 0;
 		PredXact->OldCommittedSxact->SeqNo.lastCommitBeforeSnapshot = 0;
 		SHMQueueInit(&PredXact->OldCommittedSxact->outConflicts);
@@ -1644,6 +1645,7 @@ RegisterSerializableTransactionInt(Snapshot snapshot)
 	/* Initialize the structure. */
 	sxact->vxid = vxid;
 	sxact->SeqNo.lastCommitBeforeSnapshot = PredXact->LastSxactCommitSeqNo;
+	sxact->prepareSeqNo = InvalidSerCommitSeqNo;
 	sxact->commitSeqNo = InvalidSerCommitSeqNo;
 	SHMQueueInit(&(sxact->outConflicts));
 	SHMQueueInit(&(sxact->inConflicts));
@@ -4401,18 +4403,13 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 		{
 			SERIALIZABLEXACT *t2 = conflict->sxactIn;
 
-			/*
-			 * Note that if T2 is merely prepared but not yet committed, we
-			 * rely on t->commitSeqNo being InvalidSerCommitSeqNo, which is
-			 * larger than any valid commit sequence number.
-			 */
 			if (SxactIsPrepared(t2)
 				&& (!SxactIsCommitted(reader)
-					|| t2->commitSeqNo <= reader->commitSeqNo)
+					|| t2->prepareSeqNo <= reader->commitSeqNo)
 				&& (!SxactIsCommitted(writer)
-					|| t2->commitSeqNo <= writer->commitSeqNo)
+					|| t2->prepareSeqNo <= writer->commitSeqNo)
 				&& (!SxactIsReadOnly(reader)
-			   || t2->commitSeqNo <= reader->SeqNo.lastCommitBeforeSnapshot))
+			   || t2->prepareSeqNo <= reader->SeqNo.lastCommitBeforeSnapshot))
 			{
 				failure = true;
 				break;
@@ -4453,17 +4450,11 @@ OnConflict_CheckForSerializationFailure(const SERIALIZABLEXACT *reader,
 		{
 			SERIALIZABLEXACT *t0 = conflict->sxactOut;
 
-			/*
-			 * Note that if the writer is merely prepared but not yet
-			 * committed, we rely on writer->commitSeqNo being
-			 * InvalidSerCommitSeqNo, which is larger than any valid commit
-			 * sequence number.
-			 */
 			if (!SxactIsDoomed(t0)
 				&& (!SxactIsCommitted(t0)
-					|| t0->commitSeqNo >= writer->commitSeqNo)
+					|| t0->commitSeqNo >= writer->prepareSeqNo)
 				&& (!SxactIsReadOnly(t0)
-			   || t0->SeqNo.lastCommitBeforeSnapshot >= writer->commitSeqNo))
+			   || t0->SeqNo.lastCommitBeforeSnapshot >= writer->prepareSeqNo))
 			{
 				failure = true;
 				break;
@@ -4602,6 +4593,7 @@ PreCommit_CheckForSerializationFailure(void)
 						 offsetof(RWConflictData, inLink));
 	}
 
+	MySerializableXact->prepareSeqNo = ++(PredXact->LastSxactCommitSeqNo);
 	MySerializableXact->flags |= SXACT_FLAG_PREPARED;
 
 	LWLockRelease(SerializableXactHashLock);
@@ -4776,6 +4768,7 @@ predicatelock_twophase_recover(TransactionId xid, uint16 info,
 		sxact->pid = 0;
 
 		/* a prepared xact hasn't committed yet */
+		sxact->prepareSeqNo = RecoverySerCommitSeqNo;
 		sxact->commitSeqNo = InvalidSerCommitSeqNo;
 		sxact->finishedBefore = InvalidTransactionId;
 
diff --git a/src/include/storage/predicate_internals.h b/src/include/storage/predicate_internals.h
index 0c90f27..43fcce4 100644
--- a/src/include/storage/predicate_internals.h
+++ b/src/include/storage/predicate_internals.h
@@ -57,6 +57,7 @@ typedef struct SERIALIZABLEXACT
 {
 	VirtualTransactionId vxid;	/* The executing process always has one of
 								 * these. */
+	SerCommitSeqNo prepareSeqNo;
 	SerCommitSeqNo commitSeqNo;
 	union						/* these values are not both interesting at
 								 * the same time */

--------------040703000502070501010607--