Dan Ports <drkp@csail.mit.edu> writes:
> On Wed, Oct 19, 2011 at 12:56:58PM -0400, Tom Lane wrote:
>> The proposed synchronized-snapshots feature will mean
>> that the allegedly-new snapshot actually was taken some time before,
>> so it seems to me that either this is not necessary or we cannot use
>> a synchronized snapshot in a serializable xact.
> There are definitely potential problems here. If the original snapshot
> doesn't belong to an active serializable transaction, we may have
> discarded the state we need to do SSI, e.g. we might have already
> cleaned up SIREAD locks from concurrent committed transactions.
> I assume the answer here is going to have to be to either refuse to
> start a serializable transaction if that's the case, or make saving a
> snapshot inhibit some of the SSI cleanup.
We can easily mark an exported snapshot with the IsoLevel of the
transaction that made it, and then for example refuse to adopt a
less-than-serializable snapshot into a serializable transaction.
So that aspect can be had if we need it. But we still have a race
condition with the patch as it stands, because there is a window for the
original xact to terminate before GetSerializableTransactionSnapshotInt
runs. It sounds like we have to fix that.
>> In the same vein, why is it necessary to be holding
>> SerializableXactHashLock (exclusively, yet) while acquiring the
>> snapshot? That seems rather bad from a concurrency standpoint, and
>> again it's going to be pretty meaningless if we're just installing a
>> pre-existing snapshot.
> Yes, it's bad. I'm working on a design to address
> SerializableXactHashLock contention, but there needs to be some locking
> here for the reasons I mentioned above. I think the best we can do here
> is to acquire a lock in shared mode when registering a serializable
> transaction and in exclusive mode when committing. (Which is what you'd
> expect, I guess; it's the same story as ProcArrayLock, and for most of
> the same reasons.) Obviously, we'll also want to minimize the amount of
> work we're doing while holding that lock.
I wonder whether it would be prudent to set the synchronized-snapshots
patch aside until you've finished that work (assuming you're actively
working on it). It's evidently going to require some nontrivial changes
in predicate.c, and I don't think the feature should take precedence
over SSI performance improvement.
regards, tom lane