Re: SSI patch version 12

On 15.01.2011 01:54, Kevin Grittner wrote:
> 	/*
> 	 * for r/o transactions: list of concurrent r/w transactions that we could
> 	 * potentially have conflicts with, and vice versa for r/w transactions
> 	 */
> 	TransactionId topXid;		/* top level xid for the transaction, if one
> 								 * exists; else invalid */
> 	TransactionId finishedBefore;		/* invalid means still running; else
> 										 * the struct expires when no
> 										 * serializable xids are before this. */
> 	TransactionId xmin;			/* the transaction's snapshot xmin */
> 	uint32		flags;			/* OR'd combination of values defined below */
> 	int			pid;			/* pid of associated process */
> } SERIALIZABLEXACT;

What does that comment about list of concurrent r/w transactions refer 
to? I don't see any list there. Does it refer to 
possibleUnsafeConflicts, which is above that comment?

Above SERIALIZABLEXID struct:
>  * A hash table of these objects is maintained in shared memory to provide a
>  * quick way to find the top level transaction information for a serializable
>  * transaction,  Because a serializable transaction can acquire a snapshot
>  * and read information which requires a predicate lock before it has a
>  * TransactionId, it must be keyed by VirtualTransactionId; this hashmap
>  * allows a fast link from MVCC transaction IDs to the related serializable
>  * transaction hash table entry.

I believe the comment is trying to say that there's some *other* hash 
that is keyed by VirtualTransactionId, so we need this other one keyed 
by TransactionId. It took me a while to understand that, it should be 
rephrased.

Setting the high bit in OldSetXidAdd() seems a bit weird. How about just 
using UINT64_MAX instead of 0 to mean no conflicts? Or 1, and start the 
sequence counter from 2.

ReleasePredicateLocks() reads ShmemVariableCache->nextXid without 
XidGenLock. Maybe it's safe, we assume that TransactionIds are atomic 
elsewhere, but at least there needs to be a comment explaining it. But 
it probably should use ReadNewTransactionId() instead.

Attached is a patch for some trivial changes, mostly typos.


Overall, this is very neat and well-commented code. All the data 
structures make my head spin, but I don't see anything unnecessary or 
have any suggestions for simplification. There's a few remaining TODO 
comments in the code, which obviously need to be resolved one way or 
another, but as soon as you're finished with any outstanding issues that 
you know of, I think this is ready for commit.

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