On 10.06.2011 18:05, Kevin Grittner wrote:
Heikki Linnakangas<heikki.linnakan...@enterprisedb.com> wrote:o There is no safeguard against actually wrapping around the SLRU, just the warningAny thoughts on what we should do instead? If someone holds open a transaction long enough to burn through a billion transaction IDs (or possibly less if someone uses a smaller BLCKSZ), should we generate a FATAL error?
FATAL is a bit harsh, ERROR seems more appropriate.
o I'm suspicious of the OldSerXidPagePrecedesLogically() logic with 32k BLCKSZ. In that case, OLDSERXID_MAX_PAGE is a larger than necessary to cover the whole range of 2^32 transactions, so at high XIDs, say 2^32-1, doesn't it incorrectly think that low XIDs, say 10, are in the past, not the future?I will look that over to see; but if this is broken, then one of the other SLRU users is probably broken, because I think I stole this code pretty directly from another spot.
It was copied from async.c, which doesn't have this problem because it's not mapping XIDs to the slru. There, the max queue size is determined by the *_MAX_PAGE, and you point to a particular location in the SLRU with simply page+offset.
/* * Keep a pointer to the currently-running serializable * transaction (if any) for quick reference. * TODO SSI: Remove volatile qualifier and the then-unnecessary * casts? */ static volatile SERIALIZABLEXACT *MySerializableXact = InvalidSerializableXact;* So, should we remove it? In most places where contents of MySerializableXact are modified, we're holding SerializableXactHashLock in exclusive mode. However, there's two exceptions: o GetSafeSnapshot() modifies MySerializableXact->flags without any lock. It also inspects MySerializableXact->possibleUnsafeConflicts without a lock. What if somone sets some other flag in the flags bitmap just when GetSafeSnapshot() is about to set SXACT_FLAG_DEFERRABLE_WAITING? One of the flags might be lost :-(. o CheckForSerializableConflictIn() sets SXACT_FLAG_DID_WRITE without holding a lock. The same danger is here if someone else tries to set some other flag concurrently. I think we should simply acquire SerializableXactHashLock in GetSafeSnapshot(). It shouldn't be so much of a hotspot that it would make any difference in performance. CheckForSerializableConflictIn() might be called a lot, however, so maybe we need to check if the flag is set first, and only acquire the lock and set it if it's not set already.OK. Do checks such as that argue for keeping the volatile flag, or do you think we can drop it if we make those changes? (That would also allow dropping a number of casts which exist just to avoid warnings.)
I believe we can drop it, I'll double-check.
I'm happy to work on modifications for any of this or to stay out of your way if you want to. Should I put together a patch for those items where we seem to agree and have a clear way forward?
I'll fix the MySerializableXact locking issue, and come back to the other issues on Monday. If you have the time and energy to write a patch by then, feel free, but I can look into them otherwise.
-- 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
