Re: SSI patch version 12 - Mailing list pgsql-hackers

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
> There's a few remaining TODO comments in the code, which obviously
> need to be resolved one way or another
Not all of these are "must haves" for 9.1.  Here's how they break
down:
The two in predicate_internals.h mark places which would need to be
touched if we further modify the btree AM to do index-tuple level
predicate locking.  I know that Dan was eager to tackle that because
his group at MIT has predicate locking working at that granularity
for their transaction-aware caching which works with PostgreSQL.  At
this point, though, I'm very skeptical about starting that effort
for 9.1; it seems like something for 9.2.
There's one TODO related to whether we can drop the volatile
modifier from MySerializableXact.  I've started reviewing code
around this, but am not yet done.  It wouldn't be terrible to leave
it there and worry about this for 9.2, but it will be even better if
we can clear it up one way or the other now.
Three are marking the spots we want to touch if a patch becomes
available which lets us cancel the transaction on one connection
from another.  I'd love to take care of these, but we can't without
a separate patch that I know *I'm* not in a position to write for
9.1.  We may need to leave these for 9.2, as well.
One is about the desirability of "taking some action" (probably
reporting a warning) in the SLRU summarization code if we've burned
through over a billion transaction IDs while one read write
transaction has remained active.  That puts us close to where we'd
need to start canceling attempts to start a new transaction due to
resource issues.  Seems like we should issue that warning for 9.1. 
Not big or dangerous.  I'll do it.
One is related to the heuristics for promoting the granularity of
predicate locks.  We picked numbers out of the air which seemed
reasonable at the time.  I'm sure we can make this more
sophisticated, but what we have seems to be working OK.  I'm tempted
to suggest that we leave this alone until 9.2 unless someone has a
suggestion for a particular improvement.
One is related to the number of structures to allocate for detailed
conflict checking.  We've never seen a problem with this limit, at
least that has reached me.  On the other hand, it's certainly at
least theoretically possible to hit the limit and cause confusing
errors.  Perhaps we can put this one on a GUC and make sure the
error message is clear with a hint to think about adjusting the GUC?
The GUC would be named something like
max_conflicts_per_serializable_transaction (or something to that
general effect).  We should probably do that or bump up the limit.
If my back-of-the-envelope math is right, a carefully constructed
pessimal load could need up to (max_connections / 2)^2 -- so 100
connections could conceivably require 2500 structures, although such
a scenario would be hard to create.  Current "picked from thin air"
numbers would have space for 500.  The structure is six times the
pointer size, so 24 bytes each at 32 bit and 48 bytes each at 64
bit.
Three TODOs have popped up since Heikki's post because I pushed
Dan's 2PC WIP to the repo -- it's at a point where behavior should
be right for cases where the server keeps running.  He's working on
the persistence aspect now, and there are TODOs in the new stubs
he's filling out.  Definitely 9.1.
Then there's one still lurking outside the new predicate* files, in
heapam.c.  It's about 475 lines into the heap_update function (25
lines from the bottom).  In reviewing where we needed to acquire
predicate locks, this struck me as a place where we might need to
duplicate the predicate lock from one tuple to another, but I wasn't
entirely sure.  I tried for a few hours one day to construct a
scenario which would show a serialization anomaly if I didn't do
this, and failed create one.  If someone who understands both the
patch and heapam.c wants to look at this and offer an opinion, I'd
be most grateful.  I'll take another look and see if I can get my
head around it better, but failing that, I'm disinclined to either
add locking I'm not sure we need or to remove the comment that says
we *might* need it there.
That's all of them.
-Kevin