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