Re: [HACKERS] SSI patch version 12
"Kevin Grittner" wrote: > 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. Er, actually, we would have space for 5000, because it's five times the number of SERIALIZABLEXACT structures which is ten times max_connections. I guess that would explain why I've never seen a report of a problem. Still, someone who creates a very large number of connections and pounds them heavily with SERIALIZABLE transactions might conceivably run into it. Since that's something the docs explicitly warn you *not* to do with serializable transactions, I'm not sure we need to do more than make sure the error message and hint are good. Thoughts? -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
On Tue, Jan 18, 2011 at 3:18 PM, Kevin Grittner wrote: > That's all of them. Our existing code has plenty of TODOs in it already, so I see no problem with continuing to comment places where future enhancements are possible, as long as they don't reflect deficiencies that are crippling in the present. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
Heikki Linnakangas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
"Kevin Grittner" writes: >> Dan Ports wrote: > The biggest, in my mind, is whether > MySerializableXact needs to be declared volatile. > The problem is that I don't have a very clear sense of what it really > does, which is not helped much by having done a few years of Java > programming, where the same keyword seems to have a vaguely-similar- > but-not-really-the-same meaning. In C it means that the compiler must not try to optimize away loads or stores of the variable, because the variable is subject to being read or changed by outside forces (interrupt service routines, other processes or threads, etc). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
> Dan Ports wrote: > On Mon, Jan 17, 2011 at 07:20:20PM -0600, Kevin Grittner wrote: >> OK. I may need to bounce some questions off the list to resolve >> some of them. The biggest, in my mind, is whether >> MySerializableXact needs to be declared volatile. I don't have my >> head around the issues on that as well as I might. Any thoughts on >> it? > > I'd been meaning to ask about that too. I couldn't think of any > reason it would need to be volatile. Why do you think it might need > to be? I honestly can't remember why I initially put that there -- possibly because I had a notion that some modifications to the structure might not need to be so closely synchronized with other processes as to need have modifications covered by a LW lock, but not wanting them to linger forever. It may be completely bogus. The problem is that I don't have a very clear sense of what it really does, which is not helped much by having done a few years of Java programming, where the same keyword seems to have a vaguely-similar- but-not-really-the-same meaning. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
On Mon, Jan 17, 2011 at 07:20:20PM -0600, Kevin Grittner wrote: > OK. I may need to bounce some questions off the list to resolve some > of them. The biggest, in my mind, is whether MySerializableXact > needs to be declared volatile. I don't have my head around the > issues on that as well as I might. Any thoughts on it? I'd been meaning to ask about that too. I couldn't think of any reason it would need to be volatile. Why do you think it might need to be? Dan -- Dan R. K. Ports MIT CSAILhttp://drkp.net/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
> Heikki Linnakangas wrote: > 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. Sure. I think I like reserving 1 and starting at 2 better. Will do. > 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. Hmm... We don't want to *assign* a new xid here -- we just want to know what the next one *will be*. I will review locking around this area, but I think it's sound. If that checks out OK on review, I'll add comments. > Attached is a patch for some trivial changes, mostly typos. Wow. I transpose letters a lot don't I? Thanks for finding those. Will fix. > 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. OK. I may need to bounce some questions off the list to resolve some of them. The biggest, in my mind, is whether MySerializableXact needs to be declared volatile. I don't have my head around the issues on that as well as I might. Any thoughts on it? Thanks for the multiple reviews and all the suggestions. The code is clearly better for your attention. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
On Mon, Jan 17, 2011 at 06:52:09PM -0600, Kevin Grittner wrote: > I think we still need the vxid. It shows in the pg_locks view, and > we might possibly need it to find the right process to cancel once we > have some way to do that. But there's no point with having the tag > level anymore. Oh, right, we do use it to generate pg_lock_status. I forgot about that particular tendril of code that extends outside of predicate.c. > Dan, if you're not already working on these, I'll take them, so you > can focus on 2PC. All yours! But if you feel like renaming SerializableXactHashLock, please hold off to prevent conflicts with my 2PC changes. Dan -- Dan R. K. Ports MIT CSAILhttp://drkp.net/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
> Dan Ports wrote: > Yes, that comment was supposed to be attached to > possibleUnsafeConflicts. > Actually, I think that "other" hash no longer exists > The comment above SERIALIZABLEXACT also needs to be updated since > it refers to said hash table. And if I'm not mistaken (Kevin?), we > can eliminate SERIALIZABLEXACTTAG altogether and not bother putting > the vxid in the sxact. I think we still need the vxid. It shows in the pg_locks view, and we might possibly need it to find the right process to cancel once we have some way to do that. But there's no point with having the tag level anymore. Otherwise, I agree with Dan on all counts. Dan, if you're not already working on these, I'll take them, so you can focus on 2PC. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
On Mon, Jan 17, 2011 at 09:58:44PM +0200, Heikki Linnakangas wrote: > 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? Yes, that comment was supposed to be attached to possibleUnsafeConflicts. It appears to have wandered down a couple lines, maybe during some combination of git merges and pgindent runs. > 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. Actually, I think that "other" hash no longer exists, it got replaced with a list because we weren't actually using vxid -> sxact lookup. So the comment appears to be both confusing and inaccurate and should be removed entirely, other than to note somewhere that not every SERIALIZABLEXACT will appear in SerializableXidHash because it might not have a TransactionId. The comment above SERIALIZABLEXACT also needs to be updated since it refers to said hash table. And if I'm not mistaken (Kevin?), we can eliminate SERIALIZABLEXACTTAG altogether and not bother putting the vxid in the sxact. While we're at it, it probably wouldn't hurt to rename SerializableXactHashLock to PredTranLock or something, since there's no SerializableXactHash anymore (although the lock is still being used correctly) Dan -- Dan R. K. Ports MIT CSAILhttp://drkp.net/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index 2024b48..c8078fe 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -29,7 +29,7 @@ -Serializable Snapshot Isloation +Serializable Snapshot Isolation @@ -61,9 +61,9 @@ do not conflict with locks acquired for writing data, and so reading never blocks writing and writing never blocks reading. PostgreSQL maintains this guarantee -even when providing the the strictest level of transaction +even when providing the strictest level of transaction isolation through the use of an innovative Serializable -Snapshot Isloation (SSI) level. +Snapshot Isolation (SSI) level. @@ -647,7 +647,7 @@ ERROR: could not serialize access due to read/write dependencies among transact - Don't leave connections dangling "idle in transction" longer than + Don't leave connections dangling "idle in transaction" longer than necessary. diff --git a/src/backend/storage/lmgr/README-SSI b/src/backend/storage/lmgr/README-SSI index 7eb82be..2c4de29 100644 --- a/src/backend/storage/lmgr/README-SSI +++ b/src/backend/storage/lmgr/README-SSI @@ -207,7 +207,7 @@ until one of the other transactions in the set of conflicts which could generate an anomaly has successfully committed. This is conceptually similar to how write conflicts are handled. To fully implement this guarantee there needs to be a way to roll back the -active transaciton for another process with a serialization failure +active transaction for another process with a serialization failure SQLSTATE, even if it is "idle in transaction". @@ -286,7 +286,7 @@ index which would have been included in the scan had they existed at the time. Conceptually, we want to lock the gaps between and surrounding index entries within the scanned range. -Correctness requires that any insert into an index generate a +Correctness requires that any insert into an index generates a rw-conflict with a concurrent seria
Re: [HACKERS] SSI patch version 12
Anssi Kääriäinen wrote: > I tried to break the version 11 of the patch (some of the work was > against earlier versions). In total I have used a full work day > just trying to break things, but haven't been able to find anything > after version 8. I can verify that the partial index issue is > fixed, and the count(*) performance is a lot better now. Thanks for all your testing! For the record, Dan found one more issue with the placement of our LW locking statements which provided a very small window for one process to see an uninitialized structure from another. He found it by uncommenting the '#define TEST_OLDSERXID' line (thereby forcing any terminated transaction immediately into the SLRU logic) and letting DBT-2 pound on it for a long time. If anyone else is going to be doing heavy stress testing, you might want to apply this fix: http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=036eefe05ba58da6dff9e1cae766e565182f37be The issue is sufficiently hard to hit that I didn't want to post a whole new patch for it. I've been considering it now that doc changes are complete, but am waiting to see how the 2PC work is going. > One thing I have been thinking about is how does predicate locking > indexes work when using functional indexes and functions marked as > immutable but which really aren't. I don't know how predicate > locking indexes works, so it might be that this is a non-issue. As David pointed out, doing that is likely to cause you bigger headaches than this; but yes, you can break serializability by declaring a non-immutable function as immutable and then using it on a permanent table. Like David apparently is, I'm skeptical that this merits specific mention in the docs; but I'm open to arguments to the contrary -- particularly if they suggest what language to put where. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
On Mon, Jan 17, 2011 at 09:58:35AM +0200, Anssi Kääriäinen wrote: > One thing I have been thinking about is how does predicate locking > indexes work when using functional indexes and functions marked as > immutable but which really aren't. I don't know how predicate > locking indexes works, so it might be that this is a non-issue. I > haven't been able to break anything in this way, and even if I > could, this is probably something that doesn't need anything else > than a warning that if you label your index functions immutable when > they aren't, serializable transactions might not work. When you tell the system an untruth about the state of the world, such as alleging immutability when it's not actually there, it will get its revenge in ways more drastic than this. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSI patch version 12
While I haven't tried this patch, I tried to break the version 11 of the patch (some of the work was against earlier versions). In total I have used a full work day just trying to break things, but haven't been able to find anything after version 8. I can verify that the partial index issue is fixed, and the count(*) performance is a lot better now. One thing I have been thinking about is how does predicate locking indexes work when using functional indexes and functions marked as immutable but which really aren't. I don't know how predicate locking indexes works, so it might be that this is a non-issue. I haven't been able to break anything in this way, and even if I could, this is probably something that doesn't need anything else than a warning that if you label your index functions immutable when they aren't, serializable transactions might not work. On 01/15/2011 01:54 AM, Kevin Grittner wrote: The index types other than btree don't have fine-grained support, which I don't think is a fatal defect, but it would be nice to implement. I may be able to get GiST working again this weekend in addition to the documentation work. The others might not get done for 9.1 unless someone who knows their way around the guts of those AMs can give us some advice soon I wonder if there are people using GiST and GIN indexes and serializable transactions. When upgrading to 9.1 and if there is no backwards compatibility GUC this could be a problem... The amount of users in this category is probably very low anyways, so maybe this is not an issue worth worrying about. - Anssi -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
