Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Fix memory leaks in record_out() and record_send().
- 273986bf0d39 9.3.0 cited
-
Re: foreign key locks
Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-06-27T12:40:58Z
Alvaro Herrera wrote: > here's fklocks v14, which also merges to new master as there were > several other conflicts. It passes make installcheck-world now. Recent commits broke it again, so here's a rebased v15. (No changes other than attempting to merge your work with the pg_controldata and pg_resetxlog changes and wrapping that FIXME in a comment.) Using that patch, pg_upgrade completes, but pg_dumpall fails: Upgrade Complete ---------------- Optimizer statistics are not transferred by pg_upgrade so, once you start the new server, consider running: analyze_new_cluster.sh Running this script will delete the old cluster's data files: delete_old_cluster.sh + pg_ctl start -l /home/kevin/pg/master/contrib/pg_upgrade/log/postmaster2.log -w waiting for server to start.... done server started + pg_dumpall pg_dump: Dumping the contents of table "hslot" failed: PQgetResult() failed. pg_dump: Error message from server: ERROR: MultiXactId 115 does no longer exist -- apparent wraparound pg_dump: The command was: COPY public.hslot (slotname, hubname, slotno, slotlink) TO stdout; pg_dumpall: pg_dump failed on database "regression", exiting + pg_dumpall2_status=1 + pg_ctl -m fast stop waiting for server to shut down.... done server stopped + [ -n 1 ] + echo pg_dumpall of post-upgrade database cluster failed pg_dumpall of post-upgrade database cluster failed + exit 1 make[2]: *** [check] Error 1 make[2]: Leaving directory `/home/kevin/pg/master/contrib/pg_upgrade' make[1]: *** [check-pg_upgrade-recurse] Error 2 make[1]: Leaving directory `/home/kevin/pg/master/contrib' make: *** [check-world-contrib-recurse] Error 2 Did I merge it wrong? -Kevin -
Re: foreign key locks
Alvaro Herrera <alvherre@commandprompt.com> — 2012-06-27T15:27:03Z
Excerpts from Kevin Grittner's message of mié jun 27 08:40:58 -0400 2012: > Alvaro Herrera wrote: > > > here's fklocks v14, which also merges to new master as there were > > several other conflicts. It passes make installcheck-world now. > > Recent commits broke it again, so here's a rebased v15. (No changes > other than attempting to merge your work with the pg_controldata and > pg_resetxlog changes and wrapping that FIXME in a comment.) Oooh, so sorry -- I was supposed to have git-stashed that before producing the patch. This change cannot have caused the pg_dumpall problem below though, because it only touched the multixact cache code. > Using that patch, pg_upgrade completes, but pg_dumpall fails: Hmm, something changed enough that requires more than just a code merge. I'll look into it. Thanks for the merge. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
-
Re: foreign key locks
Alvaro Herrera <alvherre@commandprompt.com> — 2012-06-29T23:17:02Z
Excerpts from Kevin Grittner's message of mié jun 27 08:40:58 -0400 2012: > Alvaro Herrera wrote: > > > here's fklocks v14, which also merges to new master as there were > > several other conflicts. It passes make installcheck-world now. > > Recent commits broke it again, so here's a rebased v15. (No changes > other than attempting to merge your work with the pg_controldata and > pg_resetxlog changes and wrapping that FIXME in a comment.) Here's v16, merged to latest stuff, before attempting to fix this: > Using that patch, pg_upgrade completes, but pg_dumpall fails: > + pg_ctl start -l > /home/kevin/pg/master/contrib/pg_upgrade/log/postmaster2.log -w > waiting for server to start.... done > server started > + pg_dumpall > pg_dump: Dumping the contents of table "hslot" failed: PQgetResult() > failed. > pg_dump: Error message from server: ERROR: MultiXactId 115 does no > longer exist -- apparent wraparound I was only testing migrating from an old version into patched, not same-version upgrades. I think I see what's happening here. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
-
Re: foreign key locks
Alvaro Herrera <alvherre@commandprompt.com> — 2012-06-29T23:26:22Z
Excerpts from Alvaro Herrera's message of vie jun 29 19:17:02 -0400 2012: > Excerpts from Kevin Grittner's message of mié jun 27 08:40:58 -0400 2012: > > Alvaro Herrera wrote: > > > > > here's fklocks v14, which also merges to new master as there were > > > several other conflicts. It passes make installcheck-world now. > > > > Recent commits broke it again, so here's a rebased v15. (No changes > > other than attempting to merge your work with the pg_controldata and > > pg_resetxlog changes and wrapping that FIXME in a comment.) > > Here's v16, merged to latest stuff, Really attached now. BTW you can get this on github: https://github.com/alvherre/postgres/tree/fklocks -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
-
Re: foreign key locks
Alvaro Herrera <alvherre@commandprompt.com> — 2012-07-05T17:02:26Z
Excerpts from Alvaro Herrera's message of vie jun 29 19:17:02 -0400 2012: > I was only testing migrating from an old version into patched, not > same-version upgrades. > > I think I see what's happening here. Okay, I have pushed the fix to github -- as I suspected, code-wise the fix was simple. I will post an updated consolidated patch later today. make check of pg_upgrade now passes for me. It would be nice that "make installcheck" would notify me that some modules' tests are being skipped ... -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
-
Re: foreign key locks
Alvaro Herrera <alvherre@commandprompt.com> — 2012-07-05T22:44:11Z
Updated patch attached. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-08-17T20:23:52Z
Excerpts from Alvaro Herrera's message of jue jul 05 18:44:11 -0400 2012: > > Updated patch attached. > Patch rebased to latest sources. This is also on Github: https://github.com/alvherre/postgres/tree/fklocks -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-08-22T21:31:28Z
Patch v19 contains some tweaks. Most notably, 1. if an Xid requests a lock A, and then a lock B which is stronger than A, then record only lock B and forget lock A. This is important for performance, because EvalPlanQual obtains ForUpdate locks on the tuples that it chases on an update chain. If EPQ was called by an update or delete, previously a MultiXact was being created. In a stock pgbench run this was causing lots of multis to be created, even when there are no FKs. This was most likely involved in the 9% performance decrease that was previously reported. 2. Save a few TransactionIdIsInProgress calls in MultiXactIdWait. We were calling it to determine how many transactions were still running, but not all callers were interested in that info. XidIsInProgress is not particularly cheap, so it seems good to skip it if possible. I didn't try to measure the difference, however. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Robert Haas <robertmhaas@gmail.com> — 2012-08-25T01:51:13Z
On Wed, Aug 22, 2012 at 5:31 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Patch v19 contains some tweaks. Most notably, > > 1. if an Xid requests a lock A, and then a lock B which is stronger than > A, then record only lock B and forget lock A. This is important for > performance, because EvalPlanQual obtains ForUpdate locks on the tuples > that it chases on an update chain. If EPQ was called by an update or > delete, previously a MultiXact was being created. > > In a stock pgbench run this was causing lots of multis to be created, > even when there are no FKs. > > This was most likely involved in the 9% performance decrease that was > previously reported. Ah-ha! Neat. I'll try to find some time to re-benchmark this during the next CF, unless you did that already. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-08-27T19:28:12Z
Excerpts from Alvaro Herrera's message of mié ago 22 17:31:28 -0400 2012: > > Patch v19 contains some tweaks. Most notably, v20 is merged to latest master; the only change other than automatic merge is to update for pg_upgrade API fixes. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-08-31T04:59:51Z
v21 is merged to latest master. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-10-11T15:36:01Z
On Friday, August 31, 2012 06:59:51 AM Alvaro Herrera wrote: > v21 is merged to latest master. Ok, I am starting to look at this. (working with a git merge of alvherre/fklocks into todays master) In a very first pass as somebody who hasn't followed the discussions in the past I took notice of the following things: * compiles and survives make check * README.tuplock jumps in quite fast without any introduction * the reasons for the redesign aren't really included in the patch but just in the - very nice - pgcon paper. * "This is a bit of a performance regression, but since we expect FOR SHARE locks to be seldom used, we don’t feel this is a serious problem." makes me a bit uneasy, will look at performance separately * Should RelationGetIndexattrBitmap check whether a unique index is referenced from a pg_constraint row? Currently we pay part of the overhead independent from the existance of foreign keys. * mode == LockTupleKeyShare, == LockTupleShare, == LockTupleUpdate in heap_lock_tuple's BeingUpdated branch look like they should be an if/else if * I don't really like HEAP_UPDATE_KEY_REVOKED as a name, what about HEAP_KEYS_UPDATED (akin to HEAP_HOT_UPDATED) * how can we get infomask2 & HEAP_UPDATE_KEY_REVOKED && infomask & HEAP_XMAX_LOCK_ONLY? * heap_lock_tuple with mode == LockTupleKeyShare && nowait looks like it would wait anyway in heap_lock_updated_tuple_rec * rename HeapSatisfiesHOTUpdate, adjust comments? * I wonder whether HeapSatisfiesHOTUpdate could be made to compute the result for key_attrs and hot_attrs at the same time, often enough they will do the same thing on the same column * you didn't start it but I find that all those l$i jump labels make the code harder to follow * shouldn't XmaxGetUpdateXid be rather called MultiXactIdGetUpdateXid or such? * /* * If the existing lock mode is identical to or weaker than the new * one, we can act as though there is no existing lock and have the * upper block handle this. */ "block"? * I am not yet sure whether the (xmax == add_to_xmax) optimization in compute_new_xmax_infomask is actually safe * ConditionalMultiXactIdWait and MultiXactIdWait should probably rely on a common implementation * I am not that convinced that moving the waiting functions away from multixact.c is a good idea, but I guess the required knowledge about lockmodes makes it hard not to do so * Haven't looked yet, but I have a slightly uneasy feeling about Hot Standby interactions. Did you look at that? * Hm? HeapTupleSatisfiesVacuum #if 0 ResetMultiHintBit(tuple, buffer, xmax, true); #endif This obviously is not a real review, but just learning what the problem & patch actually try to do. This is quite a bit to take in ;). I will let it sink in ant try to have a look at the architecture and performance next. Greetings, Andres .oO(and people call catalog timetravel complicated) -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-11T16:10:12Z
Andres Freund wrote: > On Friday, August 31, 2012 06:59:51 AM Alvaro Herrera wrote: > > v21 is merged to latest master. > Ok, I am starting to look at this. > > (working with a git merge of alvherre/fklocks into todays master) > > In a very first pass as somebody who hasn't followed the discussions in the > past I took notice of the following things: > > * compiles and survives make check Please verify src/test/isolation's make installcheck as well. > * README.tuplock jumps in quite fast without any introduction Hmm .. I'll give that a second look. Maybe some material from the pgcon paper should be added as introduction. > * "This is a bit of a performance regression, but since we expect FOR SHARE > locks to be seldom used, we don’t feel this is a serious problem." makes me a > bit uneasy, will look at performance separately Thanks. Keep in mind though that the bits we really want good performance for is FOR KEY SHARE, which is going to be used in foreign keys. FOR SHARE is staying just for hypothetical backwards compatibility. I just found that people is using it, see for example http://stackoverflow.com/a/3243083 > * Should RelationGetIndexattrBitmap check whether a unique index is referenced > from a pg_constraint row? Currently we pay part of the overhead independent > from the existance of foreign keys. Noah also suggested something more or less in the along the same lines. My answer is that I don't want to do it currently; maybe we can improve on what indexes we use later, but since this can be seen as a modularity violation, I prefer to keep it as simple as possible. > * mode == LockTupleKeyShare, == LockTupleShare, == LockTupleUpdate in > heap_lock_tuple's BeingUpdated branch look like they should be an if/else if Hm, will look at that. > * I don't really like HEAP_UPDATE_KEY_REVOKED as a name, what about > HEAP_KEYS_UPDATED (akin to HEAP_HOT_UPDATED) Thanks. > * how can we get infomask2 & HEAP_UPDATE_KEY_REVOKED && infomask & > HEAP_XMAX_LOCK_ONLY? SELECT FOR KEY UPDATE? This didn't exist initially, but previous reviewers (Noah) felt that it made sense to have it for consistency. It makes the lock conflict table make more sense, anyway. > * heap_lock_tuple with mode == LockTupleKeyShare && nowait looks like it would > wait anyway in heap_lock_updated_tuple_rec Oops. > * rename HeapSatisfiesHOTUpdate, adjust comments? Yeah. > * I wonder whether HeapSatisfiesHOTUpdate could be made to compute the result > for key_attrs and hot_attrs at the same time, often enough they will do the > same thing on the same column Hmm, I will look at that. > * you didn't start it but I find that all those l$i jump labels make the code > harder to follow You mean you suggest to have them renamed? That can be arranged. > * shouldn't XmaxGetUpdateXid be rather called MultiXactIdGetUpdateXid or such? Yes. > /* > * If the existing lock mode is identical to or weaker than the new > * one, we can act as though there is no existing lock and have the > * upper block handle this. > */ > "block"? Yeah, as in "the if {} block above". Since this is not clear maybe it needs rewording. > * I am not yet sure whether the (xmax == add_to_xmax) optimization in > compute_new_xmax_infomask is actually safe Will think about it and add a/some comment(s). > * ConditionalMultiXactIdWait and MultiXactIdWait should probably rely on a > common implementation Will look at that. > * I am not that convinced that moving the waiting functions away from > multixact.c is a good idea, but I guess the required knowledge about lockmodes > makes it hard not to do so Yeah. The line between multixact.c and heapam.c is a zigzag currently, I think. Maybe it needs to be more clear; I think the multixact modes really belong into heapam.c, and they are just uint32 to multixact.c. I wasn't brave enough to attempt this; maybe I should. > * Haven't looked yet, but I have a slightly uneasy feeling about Hot Standby > interactions. Did you look at that? > > * Hm? > HeapTupleSatisfiesVacuum > #if 0 > ResetMultiHintBit(tuple, buffer, xmax, true); > #endif Yeah. I had a couple of ResetMultiHintBit calls sprinkled in some of the tqual routines, with the idea that if they saw multis that were no longer live they could be removed, and replaced by just the remaining updater. However, this doesn't really work because it's not safe to change the Xmax when not holding exclusive lock, and the tqual routines don't know that. So we're stuck with keeping the multi in there, even when we know they are no longer interesting. This is a bit sad, because it would be a performance benefit to remove them; but it's not strictly necessary for correctness, so we can leave the optimization for a later phase. I let those #ifdefed out calls in place so that it's easy to see where the reset needs to happen. > This obviously is not a real review, but just learning what the problem & patch > actually try to do. This is quite a bit to take in ;). I will let it sink in > ant try to have a look at the architecture and performance next. Well, the patch is large and complex so any review is obviously going to take a long time. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-12T21:09:45Z
Alvaro Herrera wrote: > Andres Freund wrote: > > * heap_lock_tuple with mode == LockTupleKeyShare && nowait looks like it would > > wait anyway in heap_lock_updated_tuple_rec > > Oops. I took a stab at fixing this. However, it is not easy. First you need a way to reproduce the problem, which involves setting breakpoints in GDB. (Since a REPEATABLE READ transaction will fail to follow an update chain due to "tuple concurrently updated", you need to use a READ COMMITTED transaction; but obviously the timing to insert the bunch of updates in the middle is really short. Hence I inserted a breakpoint at the end of GetSnapshotData, had a SELECT FOR KEY SHARE NOWAIT get stuck in it, and then launched a couple of updates in another session). I was able to reproduce the undesirable wait. I quickly patched heap_lock_updated_tuple to pass down the nowait flag, but this is actually not reached, because the update chain is first followed by EvalPlanQual in ExecLockRows, and not by heap_lock_updated_tuple directly. And EPQ does not have the nowait behavior. So it still blocks. Maybe what we need to do is prevent ExecLockRows from following the update chain altogether -- after all, if heap_lock_tuple is going to do it by itself maybe it's wholly redundant. Not really sure what's the best way to approach this. At this stage I'm inclined to ignore the problem, unless some EPQ expert shows up and tells me that (1) it's okay to patch EPQ in that way, or (2) we should hack ExecLockRows (and remove EPQ?). I pushed (to github) patches for a couple of other issues you raised. Some others still need a bit more of my attention. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-18T19:58:20Z
Here is version 22 of this patch. This version contains fixes to issues reported by Andres, as well as a rebase to latest master. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Noah Misch <noah@leadboat.com> — 2012-10-19T15:40:29Z
On Thu, Oct 18, 2012 at 04:58:20PM -0300, Alvaro Herrera wrote: > Here is version 22 of this patch. This version contains fixes to issues > reported by Andres, as well as a rebase to latest master. I scanned this for obvious signs of work left to do. It contains numerous XXX and FIXME comments. Many highlight micro-optimization opportunities and the like; those can stay. Others preclude commit, either highlighting an unsolved problem or wrongly highlighting a non-problem: > + /* > + * XXX we do not lock this tuple here; the theory is that it's sufficient > + * with the buffer lock we're about to grab. Any other code must be able > + * to cope with tuple lock specifics changing while they don't hold buffer > + * lock anyway. > + */ > * so they will be uninteresting by the time our next transaction starts. > * (XXX not clear that this is correct --- other members of the MultiXact > * could hang around longer than we did. However, it's not clear what a > ! * better policy for flushing old cache entries would be.) FIXME actually > ! * this is plain wrong now that multixact's may contain update Xids. > ! nmembers = GetMultiXactIdMembers(multi, &members, true); > ! /* > ! * XXX we don't have the infomask to run the required consistency check > ! * here; the required notational overhead seems excessive. > ! */ > /* We assume the cache entries are sorted */ > ! /* XXX we assume the unused bits in "status" are zeroed */ > ! if (memcmp(members, entry->members, nmembers * sizeof(MultiXactMember)) == 0) > ! * XXX do we have any issues with needing to checkpoint here? > */ > ! static void > ! TruncateMultiXact(void) > { > + /* FIXME what should we initialize this to? */ > + newFrozenMulti = ReadNextMultiXactId(); > + * FIXME -- the XMAX_IS_MULTI test is a bit wrong .. it's possible to > + * have tuples with that bit set that are dead. However, if that's > + * changed, the RawXmax() call below should probably be researched as well. > */ > if (tuple->t_infomask & > ! (HEAP_XMAX_INVALID | HEAP_XMAX_LOCK_ONLY | HEAP_XMAX_IS_MULTI)) > return false; -
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-10-26T22:06:53Z
On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote: > Here is version 22 of this patch. This version contains fixes to issues > reported by Andres, as well as a rebase to latest master. Ok, I now that pgconf.eu has ended I am starting to do a real review: * Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and earlier you could be sure that if you FOR UPDATE'ed a row you could delete it. Unless I miss something now this will not block somebody else acquiring a FOR KEY SHARE lock, so this guarantee is gone. This seems likely to introduce subtle problems in user applications. I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of FOR UPDATE. You write "SELECT FOR UPDATE is a standards-compliant exclusive lock". I didn't really find all that much about the semantics of FOR UPDATE on cursors in the standard other than "The operations of update and delete are permitted for updatable cursors, subject to constraining Access Rules.". * I would welcome adding some explanatory comments about the point of TupleLockExtraInfo and MultiXactStatusLock at the respective definition. * Why do we have the HEAP_XMAX_IS_MULTI && HEAP_XMAX_LOCK_ONLY case? * I think some of the longer comments could use the attention of a native speaker, unfortunately thats not me. * I am still uncomfortable with the FOR SHARE deoptimization. I have seen people lock larger parts of their table to make some READ COMMITTED things actually safe. Is there any problem retaining the non XMAX_IS_MULTI behaviour except space in infomask? That seems solveable by something like #define HEAP_XMAX_SHR_LOCK 0x0010 #define HEAP_XMAX_EXCL_LOCK 0x0040 #define HEAP_XMAX_KEYSHR_LOCK (HEAP_XMAX_SHR_LOCK | HEAP_XMAX_EXCL_LOCK) and somewhat more complex expressions when testing the locks ((infomask & HEAP_XMAX_KEYSHR_LOCK ) == HEAP_XMAX_KEYSHR_LOCK, etc). * In heap_lock_tuple's XMAX_IS_MULTI case for (i = 0; i < nmembers; i++) { if (TransactionIdIsCurrentTransactionId(members[i].xid)) { LockTupleMode membermode; membermode = TUPLOCK_from_mxstatus(members[i].status); if (membermode > mode) { if (have_tuple_lock) UnlockTupleTuplock(relation, tid, mode); pfree(members); return HeapTupleMayBeUpdated; } } } why is it membermode > mode and not membermode >= mode? * Is the case of a a non-key-update followed by a key-update actually handled when doing a heap_lock_tuple with mode = LockTupleKeyShare and follow_updates = false? I don't really see how, so it seems to deserve at least a comment. But then I don't yet understand why follow_update makes sense. * In heap_lock_tuple with mode == LockTupleUpdate && infomask & HEAP_XMAX_IS_MULTI, were leaking members when doing goto l3. Probably not relevant, but given the code tries to be careful everywhere else... We might also leak in the members == 0 case there, not sure yet. Ok, this is at about 35% of the diff in my second pass, but I just arrived back in Berlin, and this seems useful enough on its own... Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: foreign key locks
Simon Riggs <simon@2ndquadrant.com> — 2012-10-28T22:47:16Z
On 27 October 2012 00:06, Andres Freund <andres@2ndquadrant.com> wrote: > On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote: >> Here is version 22 of this patch. This version contains fixes to issues >> reported by Andres, as well as a rebase to latest master. > > Ok, I now that pgconf.eu has ended I am starting to do a real review: > > * Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and earlier > you could be sure that if you FOR UPDATE'ed a row you could delete it. Unless > I miss something now this will not block somebody else acquiring a FOR KEY > SHARE lock, so this guarantee is gone. Yes, that is exactly the point of this. > This seems likely to introduce subtle problems in user applications. Yes, it could. So we need some good docs on explaining this. Which applications use FOR UPDATE? Any analysis of particular situations would be very helpful in doing the correct thing here. I think introducing FOR DELETE would be much clearer as an addition/ synonym for FOR KEY UPDATE. > I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR > UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of FOR > UPDATE. Which is essentially unwinding the feature, to some extent. Does FOR UPDATE throw an error if the user later issues an UPDATE of the PK or a DELETE? That sequence of actions would cause lock escalation in the application, which could also lead to deadlock/contention. This sounds like we need a GUC or table-level default to control whether FOR UPDATE means FOR UPDATE or FOR DELETE More thought/input required on this point, it seems important. > You write "SELECT FOR UPDATE is a standards-compliant exclusive lock". I > didn't really find all that much about the semantics of FOR UPDATE on cursors > in the standard other than "The operations of update and delete are permitted > for updatable cursors, subject to constraining Access Rules.". > * I think some of the longer comments could use the attention of a native > speaker, unfortunately thats not me. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-10-29T11:27:29Z
On Sunday, October 28, 2012 11:47:16 PM Simon Riggs wrote: > On 27 October 2012 00:06, Andres Freund <andres@2ndquadrant.com> wrote: > > On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote: > >> Here is version 22 of this patch. This version contains fixes to issues > >> reported by Andres, as well as a rebase to latest master. > > > > Ok, I now that pgconf.eu has ended I am starting to do a real review: > > > > * Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and > > earlier you could be sure that if you FOR UPDATE'ed a row you could > > delete it. Unless I miss something now this will not block somebody else > > acquiring a FOR KEY SHARE lock, so this guarantee is gone. > > Yes, that is exactly the point of this. Yes, sure. The point is the introduction of a weaker lock level which can be used by the ri triggers. I don't see any imperative that the semantics of the old lock level need to be redefined. That just seems dangerous to me. We need to take care to reduce the complications of upgrades not introduce changes that require complex code reviews. > > This seems likely to introduce subtle problems in user applications. > > Yes, it could. So we need some good docs on explaining this. > > Which applications use FOR UPDATE? Any that want to protect themselves against deadlocks and/or visibility problems due to READ COMMITTED. Thats quite a bunch. > Any analysis of particular situations would be very helpful in doing > the correct thing here. Usual things include * avoiding problems due to lock upgrades in combination with foreign keys (as far as I can see some of those still persist). * prevent rows being deleted by other transactions * prepare for updating if computation of the new values take some time * guarantee order of locking to make sure rows are DELETE/UPDATEed in the same order (still no ORDER BY in UPDATE/DELETE) ... > I think introducing FOR DELETE would be much clearer as an addition/ > synonym for FOR KEY UPDATE. Hm. Not really convinced. For me that seems to just make the overall situation even more complex. > > I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR > > UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of > > FOR UPDATE. > > Which is essentially unwinding the feature, to some extent. Why? The overall features available are just the same? The only thing is that existing semantics aren't changed. > Does FOR UPDATE throw an error if the user later issues an UPDATE of > the PK or a DELETE? That sequence of actions would cause lock > escalation in the application, which could also lead to > deadlock/contention. Unless I miss something it precisely does *not* result in lock escalation with the 9.2 semanticsbut it *would* with fklocks applied. Thats exactly my point. > This sounds like we need a GUC or table-level default to control > whether FOR UPDATE means FOR UPDATE or FOR DELETE I don't like adding a new guc for something that should be solveable with some creative naming. If a new user doesn't get a bit more concurrency due to manually issued 9.2 FOR UPDATE implicitly being converted into a FOR NO KEY UPDATE its not too bad. The code needs to be checked whether thats valid anyway. The reverse is not true... > More thought/input required on this point, it seems important. Yep, more input welcome. Greetings, Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Simon Riggs <simon@2ndquadrant.com> — 2012-10-29T12:08:16Z
On 29 October 2012 12:27, Andres Freund <andres@2ndquadrant.com> wrote: >> This sounds like we need a GUC or table-level default to control >> whether FOR UPDATE means FOR UPDATE or FOR DELETE > > I don't like adding a new guc for something that should be solveable with some > creative naming. If a new user doesn't get a bit more concurrency due to > manually issued 9.2 FOR UPDATE implicitly being converted into a FOR NO KEY > UPDATE its not too bad. The code needs to be checked whether thats valid > anyway. The reverse is not true... The main point here is that introducing new non-optional behaviour is a compatibility problem and we shouldn't rush that. If we decide to change FOR UPDATE to mean FOR NO KEY UPDATE that needs separate consideration and shouldn't happen until sometime after the feature goes in (months, or perhaps releases). We're introducing a new feature that will allow us to avoid lock problems, by taking the current FOR UPDATE behaviour and splitting it into two options: one weaker and one stronger. We need explicit names for both of those options. The most obvious naming is FOR [[NO] KEY] UPDATE Don't much like that, but its pretty unambiguous and fairly neat. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-14T15:23:47Z
Here's version 23 of this patch, with fixes for the below comments and a merge to master. Andres Freund wrote: > On Thursday, October 18, 2012 09:58:20 PM Alvaro Herrera wrote: > > Here is version 22 of this patch. This version contains fixes to issues > > reported by Andres, as well as a rebase to latest master. > > Ok, I now that pgconf.eu has ended I am starting to do a real review: > > * Is it ok to make FOR UPDATE somewhat weaker than before? In 9.2 and earlier > you could be sure that if you FOR UPDATE'ed a row you could delete it. Unless > I miss something now this will not block somebody else acquiring a FOR KEY > SHARE lock, so this guarantee is gone. > This seems likely to introduce subtle problems in user applications. > > I propose renaming FOR UPDATE => FOR NO KEY UPDATE, FOR KEY UPDATE => FOR > UPDATE or similar (or PREVENT KEY UPDATE?). That keeps the old meaning of FOR > UPDATE. Okay, in subsequent discussion everyone agreed that this is necessary; thanks for noticing the problem. Instead of your suggestion, however, I used Noah's; luckily I just noticed that it was identical to yours. So apparently there is also agreement on this point. I have updated the code, comments and docs (and renamed some other stuff to match). > You write "SELECT FOR UPDATE is a standards-compliant exclusive lock". I > didn't really find all that much about the semantics of FOR UPDATE on cursors > in the standard other than "The operations of update and delete are permitted > for updatable cursors, subject to constraining Access Rules.". I don't really know where that comes from, but it's a statement I grabbed from the docs somewhere. > * I would welcome adding some explanatory comments about the point of > TupleLockExtraInfo and MultiXactStatusLock at the respective definition. Done. If that's not enough, I would welcome suggestions or specific question needing clarification. > * Why do we have the HEAP_XMAX_IS_MULTI && HEAP_XMAX_LOCK_ONLY case? For pg_upgrade. HEAP_XMAX_LOCK_ONLY is the value previously used by HEAP_XMAX_SHARED_LOCK, so a database containing such values that is upgraded will contain that bit pattern. > * I think some of the longer comments could use the attention of a native > speaker, unfortunately thats not me. Sorry about that. > * I am still uncomfortable with the FOR SHARE deoptimization. I have seen > people lock larger parts of their table to make some READ COMMITTED things > actually safe. I haven't changed this yet. There are bits available in infomask2 that we could use, if we agree that it is necessary; or, as you say, we could use two bits to distinguish three different states, but we need to ensure that pg_upgraded databases will work sanely. > * In heap_lock_tuple's XMAX_IS_MULTI case > > [snip] > > why is it membermode > mode and not membermode >= mode? Uh, that's a bug. Fixed. As noticed in the comment above that snippet, there was a deadlock possible here. Maybe I should add a test to ensure this doesn't happen. > * Is the case of a a non-key-update followed by a key-update actually handled > when doing a heap_lock_tuple with mode = LockTupleKeyShare and follow_updates > = false? I don't really see how, so it seems to deserve at least a comment. I wasn't able to figure out what you think is the problem. > But then I don't yet understand why follow_update makes sense. Basically heap_lock_tuple can be told by the caller to follow the update chain to lock subsequent tuples, or not. Mainly, EvalPlanQual does not want this to happen, because it does its own update-chain walking. In all honesty, it is not clear to me that this is the proper solution to the problem. Maybe some hacking around ExecLockRows is necessary. > * In heap_lock_tuple with mode == LockTupleUpdate && infomask & > HEAP_XMAX_IS_MULTI, were leaking members when doing goto l3. Probably not > relevant, but given the code tries to be careful everywhere else... Right, plugged. > We might also leak in the members == 0 case there, not sure yet. Hm, I don't think GetMultiXactIdMembers really considers the case when 0 members are to be returned. Maybe that needs some more tweaking. > Ok, this is at about 35% of the diff in my second pass, but I just arrived back > in Berlin, and this seems useful enough on its own... Many thanks. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-14T16:27:26Z
Alvaro Herrera wrote: > > * In heap_lock_tuple's XMAX_IS_MULTI case > > > > [snip] > > > > why is it membermode > mode and not membermode >= mode? > > Uh, that's a bug. Fixed. As noticed in the comment above that snippet, > there was a deadlock possible here. Maybe I should add a test to ensure > this doesn't happen. Done: https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770 (I don't think this is worth a v24 submission). -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Noah Misch <noah@leadboat.com> — 2012-11-16T00:24:59Z
On Wed, Nov 14, 2012 at 01:27:26PM -0300, Alvaro Herrera wrote: > https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770 > > (I don't think this is worth a v24 submission). Are you aware of any defects in or unanswered questions of this version that would stall your commit thereof?
-
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-11-16T14:55:50Z
On 2012-11-14 13:27:26 -0300, Alvaro Herrera wrote: > Alvaro Herrera wrote: > > > > * In heap_lock_tuple's XMAX_IS_MULTI case > > > > > > [snip] > > > > > > why is it membermode > mode and not membermode >= mode? > > > > Uh, that's a bug. Fixed. As noticed in the comment above that snippet, > > there was a deadlock possible here. Maybe I should add a test to ensure > > this doesn't happen. > > Done: > https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770 > > (I don't think this is worth a v24 submission). I have started doing some performance testing and I fear I was right in being suspicious about the performance difference for FOR SHARE locks: Tested with pgbench -p 5442 -U andres \ -c 30 -j 30 \ -M prepared -f ~/tmp/postgres-fklocks/select-for-share.sql \ -T 20 postgres on a pgbench -i -s 10 database, where select-for-share.sql is: BEGIN; \set naccounts 1000000 \setrandom aid 1 :naccounts SELECT abalance FROM pgbench_accounts WHERE aid = :aid FOR SHARE; \setrandom aid 1 :naccounts SELECT abalance FROM pgbench_accounts WHERE aid = :aid FOR SHARE; \setrandom aid 1 :naccounts SELECT abalance FROM pgbench_accounts WHERE aid = :aid FOR SHARE; COMMIT; which very roughly resembles workloads I have seen in reality (locking some records your rely on while you do some work). With 52b4729fcfc20f056f17531a6670d8c4b9696c39 (alvherre/fklocks) vs 273986bf0d39e5166eb15ba42ebff4803e23a688 (latest merged master) I get tps = 8986.133496 (excluding connections establishing) vs tps = 25307.861193 (excluding connections establishing) Thats nearly a factor of three which seems to be too big to be acceptable to me. So I really think we need to bring FOR SHARE locks back as a flag. I have done some benchmarking of other cases (plain pgbench, pgbench with foreign keys, large insertions, large amounts of FOR SHARE locks) and haven't found anything really outstanding so far. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-16T16:17:47Z
Noah Misch wrote: > On Wed, Nov 14, 2012 at 01:27:26PM -0300, Alvaro Herrera wrote: > > https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770 > > > > (I don't think this is worth a v24 submission). > > Are you aware of any defects in or unanswered questions of this version that > would stall your commit thereof? Yeah, I am revisiting the list of XXX/FIXME comments you pointed out previously. And I would still like someone with EPQ understanding to review the ExecLockRows / EvalPlanQual / heap_lock_tuple interactions. Andres is on the verge of convincing me that we need to support singleton FOR SHARE without multixacts due to performance concerns. It would be useful for more people to chime in here: is FOR SHARE an important case to cater for? I wonder if using FOR KEY SHARE (keep performance characteristics, but would need to revise application code) would satisfy Andres' users, for example. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-11-16T16:31:12Z
On 2012-11-16 13:17:47 -0300, Alvaro Herrera wrote: > Noah Misch wrote: > > On Wed, Nov 14, 2012 at 01:27:26PM -0300, Alvaro Herrera wrote: > > > https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770 > > > > > > (I don't think this is worth a v24 submission). > > > > Are you aware of any defects in or unanswered questions of this version that > > would stall your commit thereof? > > Yeah, I am revisiting the list of XXX/FIXME comments you pointed out > previously. > > And I would still like someone with EPQ understanding to review the > ExecLockRows / EvalPlanQual / heap_lock_tuple interactions. I am in the process of looking at those atm, but we need somebody that actually understands the intricacies here... > Andres is on the verge of convincing me that we need to support > singleton FOR SHARE without multixacts due to performance concerns. I don't really see any arguments against doing so. We aren't in a that big shortage of flags and if we need more than available I think we can free some (e.g. XMAX/XMIN_INVALID). > It > would be useful for more people to chime in here: is FOR SHARE an > important case to cater for? I wonder if using FOR KEY SHARE (keep > performance characteristics, but would need to revise application code) > would satisfy Andres' users, for example. It definitely wouldn't help in the cases I have seen because the point is to protect against actual content changes of the rows, not just the keys. Note that you actually need to use explicit FOR SHARE/UPDATE for correctness purposes in many scenarios unless youre running in 9.1+ serializable mode. And that cannot be used in some cases (try queuing for example) because the rollback rates would be excessive. Even if applications could be fixed, requiring changes to applications locking behaviour, which possibly is far from trivial, seems like a too big upgrade barrier. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Noah Misch <noah@leadboat.com> — 2012-11-17T03:31:51Z
On Fri, Nov 16, 2012 at 05:31:12PM +0100, Andres Freund wrote: > On 2012-11-16 13:17:47 -0300, Alvaro Herrera wrote: > > Andres is on the verge of convincing me that we need to support > > singleton FOR SHARE without multixacts due to performance concerns. > > I don't really see any arguments against doing so. We aren't in a that > big shortage of flags and if we need more than available I think we can > free some (e.g. XMAX/XMIN_INVALID). The patch currently leaves two unallocated bits. Reclaiming currently-used bits means a binary compatibility break. Until we plan out such a thing, reclaimable bits are not as handy as never-allocated bits. I don't think we should lightly expend one of the final two. > > It > > would be useful for more people to chime in here: is FOR SHARE an > > important case to cater for? I wonder if using FOR KEY SHARE (keep > > performance characteristics, but would need to revise application code) > > would satisfy Andres' users, for example. > > It definitely wouldn't help in the cases I have seen because the point > is to protect against actual content changes of the rows, not just the > keys. > Note that you actually need to use explicit FOR SHARE/UPDATE for > correctness purposes in many scenarios unless youre running in 9.1+ > serializable mode. And that cannot be used in some cases (try queuing > for example) because the rollback rates would be excessive. I agree that tripling FOR SHARE cost is risky. Where is the added cost concentrated? Perchance that multiple belies optimization opportunities. Thanks, nm
-
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-11-17T14:20:20Z
On 2012-11-16 22:31:51 -0500, Noah Misch wrote: > On Fri, Nov 16, 2012 at 05:31:12PM +0100, Andres Freund wrote: > > On 2012-11-16 13:17:47 -0300, Alvaro Herrera wrote: > > > Andres is on the verge of convincing me that we need to support > > > singleton FOR SHARE without multixacts due to performance concerns. > > > > I don't really see any arguments against doing so. We aren't in a that > > big shortage of flags and if we need more than available I think we can > > free some (e.g. XMAX/XMIN_INVALID). > > The patch currently leaves two unallocated bits. Reclaiming currently-used > bits means a binary compatibility break. Until we plan out such a thing, > reclaimable bits are not as handy as never-allocated bits. I don't think we > should lightly expend one of the final two. Not sure what you mean with a binary compatibility break? pg_upgrade'ability? What I previously suggested somewhere was: #define HEAP_XMAX_SHR_LOCK 0x0010 #define HEAP_XMAX_EXCL_LOCK 0x0040 #define HEAP_XMAX_KEYSHR_LOCK (HEAP_XMAX_SHR_LOCK|HEAP_XMAX_EXCL_LOCK) /* * Different from _LOCK_BITS because it doesn't include LOCK_ONLY */ #define HEAP_LOCK_MASK (HEAP_XMAX_SHR_LOCK|HEAP_XMAX_EXCL_LOCK) #define HEAP_XMAX_IS_SHR_LOCKED(tup) \ (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_SHR_LOCK) #define HEAP_XMAX_IS_EXCL_LOCKED(tup) \ (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_EXCL_LOCK) #define HEAP_XMAX_IS_KEYSHR_LOCKED(tup) \ (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_KEYSHR_LOCK) It makes checking for locks sightly more more complicated, but its not too bad... > > > It > > > would be useful for more people to chime in here: is FOR SHARE an > > > important case to cater for? I wonder if using FOR KEY SHARE (keep > > > performance characteristics, but would need to revise application code) > > > would satisfy Andres' users, for example. > > > > It definitely wouldn't help in the cases I have seen because the point > > is to protect against actual content changes of the rows, not just the > > keys. > > Note that you actually need to use explicit FOR SHARE/UPDATE for > > correctness purposes in many scenarios unless youre running in 9.1+ > > serializable mode. And that cannot be used in some cases (try queuing > > for example) because the rollback rates would be excessive. > > I agree that tripling FOR SHARE cost is risky. Where is the added cost > concentrated? Perchance that multiple belies optimization opportunities. Good question, let me play a bit. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: foreign key locks
Noah Misch <noah@leadboat.com> — 2012-11-17T14:56:06Z
On Sat, Nov 17, 2012 at 03:20:20PM +0100, Andres Freund wrote: > On 2012-11-16 22:31:51 -0500, Noah Misch wrote: > > On Fri, Nov 16, 2012 at 05:31:12PM +0100, Andres Freund wrote: > > > On 2012-11-16 13:17:47 -0300, Alvaro Herrera wrote: > > > > Andres is on the verge of convincing me that we need to support > > > > singleton FOR SHARE without multixacts due to performance concerns. > > > > > > I don't really see any arguments against doing so. We aren't in a that > > > big shortage of flags and if we need more than available I think we can > > > free some (e.g. XMAX/XMIN_INVALID). > > > > The patch currently leaves two unallocated bits. Reclaiming currently-used > > bits means a binary compatibility break. Until we plan out such a thing, > > reclaimable bits are not as handy as never-allocated bits. I don't think we > > should lightly expend one of the final two. > > Not sure what you mean with a binary compatibility break? > pg_upgrade'ability? Yes. If we decide HEAP_XMIN_INVALID isn't helping, we can stop adding it to tuples anytime. Old tuples may continue to carry the bit, with no particular deadline for their demise. To reuse that bit in the mean time, we'll need to prove that no tuple writable by PostgreSQL 8.3+ will get an unacceptable interpretation under the new meaning of the bit. Alternately, build the mechanism to prove that all such old bits are gone before using the bit in the new way. This keeps HEAP_MOVED_IN and HEAP_MOVED_OFF unavailable today. > What I previously suggested somewhere was: > > #define HEAP_XMAX_SHR_LOCK 0x0010 > #define HEAP_XMAX_EXCL_LOCK 0x0040 > #define HEAP_XMAX_KEYSHR_LOCK (HEAP_XMAX_SHR_LOCK|HEAP_XMAX_EXCL_LOCK) > /* > * Different from _LOCK_BITS because it doesn't include LOCK_ONLY > */ > #define HEAP_LOCK_MASK (HEAP_XMAX_SHR_LOCK|HEAP_XMAX_EXCL_LOCK) > > #define HEAP_XMAX_IS_SHR_LOCKED(tup) \ > (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_SHR_LOCK) > #define HEAP_XMAX_IS_EXCL_LOCKED(tup) \ > (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_EXCL_LOCK) > #define HEAP_XMAX_IS_KEYSHR_LOCKED(tup) \ > (((tup)->t_infomask & HEAP_LOCK_BITS) == HEAP_XMAX_KEYSHR_LOCK) > > It makes checking for locks sightly more more complicated, but its not > too bad... Agreed; that seems reasonable.
-
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-11-17T16:07:18Z
> > I agree that tripling FOR SHARE cost is risky. Where is the added cost > > concentrated? Perchance that multiple belies optimization opportunities. > > Good question, let me play a bit. Ok, I benchmarked around and from what I see there is no single easy target. The biggest culprits I could find are: 1. higher amount of XLogInsert calls per transaction (visible in pgbench -t instead of -T mode while watching the WAL volume) 2. Memory allocations in GetMultiXactIdMembers 3. Memory allocations in mXactCachePut a) cache entry itself b) the cache context 4. More lwlocks acquisitions We can possibly optimize a bit with 2) by using a static buffer for common member sizes, but thats not going to buy us too much... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Noah Misch <noah@leadboat.com> — 2012-11-17T18:25:07Z
On Sat, Nov 17, 2012 at 05:07:18PM +0100, Andres Freund wrote: > > > I agree that tripling FOR SHARE cost is risky. Where is the added cost > > > concentrated? Perchance that multiple belies optimization opportunities. > > > > Good question, let me play a bit. > > Ok, I benchmarked around and from what I see there is no single easy > target. > The biggest culprits I could find are: > 1. higher amount of XLogInsert calls per transaction (visible > in pgbench -t instead of -T mode while watching the WAL volume) > 2. Memory allocations in GetMultiXactIdMembers > 3. Memory allocations in mXactCachePut > a) cache entry itself > b) the cache context > 4. More lwlocks acquisitions > > We can possibly optimize a bit with 2) by using a static buffer for > common member sizes, but thats not going to buy us too much... In that case, +1 for your proposal to prop up FOR SHARE.
-
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-11-19T11:58:04Z
On 2012-11-14 13:27:26 -0300, Alvaro Herrera wrote: > Alvaro Herrera wrote: > > > > * In heap_lock_tuple's XMAX_IS_MULTI case > > > > > > [snip] > > > > > > why is it membermode > mode and not membermode >= mode? > > > > Uh, that's a bug. Fixed. As noticed in the comment above that snippet, > > there was a deadlock possible here. Maybe I should add a test to ensure > > this doesn't happen. > > Done: > https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770 Some more review bits, based on ffd6250d1d393f2ecb9bfc55c2c6f715dcece557 - if oldestMultiXactId + db is set and then that database is dropped we seem to have a problem because MultiXactAdvanceOldest won't overwrite those values. Should probably use SetMultiXactIdLimit directly. - what stop multixacts only being filled out (i.e RecordNewMultiXact()) *after* the XLogInsert() *and* after a MultiXactGetCheckptMulti()? Afaics MultiXactGenLock is not hold in CreateMultiXactId(). If we crash in that moment we loose the multixact data which now means potential data loss... - multixact member group data crossing 512 sector boundaries makes me uneasy (as its 5 bytes). I don't really see a scenario where its dangerous, but ... Does anybody see a problem here? - there are quite some places that do multiStopLimit = multiWrapLimit - 100; if (multiStopLimit < FirstMultiXactId) multiStopLimit -= FirstMultiXactId; perhaps MultiXactIdAdvance and MultiXactIdRetreat macros are in order? - I find using a default: clause in switches with enum types where everything is expected to be handled like the following a bad idea, this way the compiler won't warn you if youve missed case's which makes changing/extending code harder: switch (rc->strength) { case LCS_FORNOKEYUPDATE: newrc->markType = ROW_MARK_EXCLUSIVE; break; case LCS_FORSHARE: newrc->markType = ROW_MARK_SHARE; break; case LCS_FORKEYSHARE: newrc->markType = ROW_MARK_KEYSHARE; break; case LCS_FORUPDATE: newrc->markType = ROW_MARK_KEYEXCLUSIVE; break; default: elog(ERROR, "unsupported rowmark type %d", rc->strength); } - #if 0 /* * The idea here is to remove the IS_MULTI bit, and replace the * xmax with the updater's Xid. However, we can't really do it: * modifying the Xmax is not allowed under our buffer locking * rules, unless we have an exclusive lock; but we don't know that * we have it. So the multi needs to remain in place :-( */ ResetMultiHintBit(tuple, buffer, xmax, true); #endif Three things: - HeapTupleSatisfiesUpdate is actually always called exclusively locked ;) - Extending something like LWLockHeldByMe to also return the current lockmode doesn't sound hard - we seem to be using #ifdef NOT_YET for such cases - Using a separate production for the lockmode seems to be nicer imo, not really important though for_locking_item: FOR UPDATE locked_rels_list opt_nowait ... | FOR NO KEY UPDATE locked_rels_list opt_nowait ... | FOR SHARE locked_rels_list opt_nowait ... | FOR KEY SHARE locked_rels_list opt_nowait ; - not really padding, MultiXactStatus is 4bytes... /* * XXX Note: there's a lot of padding space in MultiXactMember. We could * find a more compact representation of this Xlog record -- perhaps all the * status flags in one XLogRecData, then all the xids in another one? Not * clear that it's worth the trouble though. */ - why #define SizeOfMultiXactCreate (offsetof(xl_multixact_create, nmembers) + sizeof(int32)) and not #define SizeOfMultiXactCreate offsetof(xl_multixact_create, members) - starting a critical section in GetNewMultiXactId but not ending it is ugly, but not new Greetings, Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-11-19T12:12:25Z
On 2012-11-14 13:27:26 -0300, Alvaro Herrera wrote: > Alvaro Herrera wrote: > > > > * In heap_lock_tuple's XMAX_IS_MULTI case > > > > > > [snip] > > > > > > why is it membermode > mode and not membermode >= mode? > > > > Uh, that's a bug. Fixed. As noticed in the comment above that snippet, > > there was a deadlock possible here. Maybe I should add a test to ensure > > this doesn't happen. > > Done: > https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770 > > (I don't think this is worth a v24 submission). One more observation: /* * Get and lock the updated version of the row; if fail, return NULL. */ - copyTuple = EvalPlanQualFetch(estate, relation, LockTupleExclusive, + copyTuple = EvalPlanQualFetch(estate, relation, LockTupleNoKeyExclusive, That doesn't seem to be correct to me. Why is it ok to acquire a potentially too low locklevel here? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-20T20:36:14Z
Andres Freund wrote: > - I find using a default: clause in switches with enum types where everything > is expected to be handled like the following a bad idea, this way the > compiler won't warn you if youve missed case's which makes changing/extending code harder: > switch (rc->strength) > { I eliminated some of these default clauses, but the compiler is not happy about not having some of them, so they remain. > - > #if 0 > /* > * The idea here is to remove the IS_MULTI bit, and replace the > * xmax with the updater's Xid. However, we can't really do it: > * modifying the Xmax is not allowed under our buffer locking > * rules, unless we have an exclusive lock; but we don't know that > * we have it. So the multi needs to remain in place :-( > */ > ResetMultiHintBit(tuple, buffer, xmax, true); > #endif > > Three things: > - HeapTupleSatisfiesUpdate is actually always called exclusively locked ;) > - Extending something like LWLockHeldByMe to also return the current > lockmode doesn't sound hard > - we seem to be using #ifdef NOT_YET for such cases After spending some time trying to make this work usefully, I observed that it's pointless, at least if we apply it only in HeapTupleSatisfiesUpdate, because the IS_MULTI bit is going to be removed by compute_new_xmax_infomask if the multi is gone. Something like this would be useful if we could do it in other places; say when we're merely scanning page without the specific intention of modifying that particular tuple. Maybe in heap_prune_page, for example. ISTM we can see that as an optimization we can add later. For the record, the implementation of ResetMultiHintBit looks like this: /* * When a tuple is found to be marked by a MultiXactId, all whose members are * completed transactions, the HEAP_XMAX_IS_MULTI bit can be removed. * Additionally, at the request of caller, we can set the Xmax to the given * Xid, and set HEAP_XMAX_COMMITTED. * * Note that per buffer access rules, the caller to this function must hold * exclusive content lock on the affected buffer. */ static inline void ResetMultiHintBit(HeapTupleHeader tuple, Buffer buffer, TransactionId xid, bool mark_committed) { Assert(LWLockModeHeld((&BufferDescriptors[buffer])->content_lock == LW_EXCLUSIVE)); Assert(tuple->t_infomask & HEAP_XMAX_IS_MULTI); Assert(!(tuple->t_infomask & HEAP_XMAX_INVALID)); Assert(!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple))); tuple->t_infomask &= ~HEAP_XMAX_BITS; HeapTupleHeaderSetXmax(tuple, xid); if (!TransactionIdIsValid(xid)) tuple->t_infomask |= HEAP_XMAX_INVALID; /* note that HEAP_KEYS_UPDATED persists, if set */ if (mark_committed) SetHintBits(tuple, buffer, HEAP_XMAX_COMMITTED, xid); else SetBufferCommitInfoNeedsSave(buffer); } (This is removed by commit 52f86f82fb3e01a6ed0b8106bac20f319bb3ad0a in my github tree, but that commit might be lost in the future, hence this paste.) Some of your other observations have been fixed by commits that I have pushed to my github tree. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: foreign key locks
Andres Freund <andres@2ndquadrant.com> — 2012-11-20T21:15:33Z
On 2012-11-20 17:36:14 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > > > - I find using a default: clause in switches with enum types where everything > > is expected to be handled like the following a bad idea, this way the > > compiler won't warn you if youve missed case's which makes changing/extending code harder: > > switch (rc->strength) > > { > > I eliminated some of these default clauses, but the compiler is not > happy about not having some of them, so they remain. You have removed the ones that looked removable to me... > > > - > > #if 0 > > /* > > * The idea here is to remove the IS_MULTI bit, and replace the > > * xmax with the updater's Xid. However, we can't really do it: > > * modifying the Xmax is not allowed under our buffer locking > > * rules, unless we have an exclusive lock; but we don't know that > > * we have it. So the multi needs to remain in place :-( > > */ > > ResetMultiHintBit(tuple, buffer, xmax, true); > > #endif > > > > Three things: > > - HeapTupleSatisfiesUpdate is actually always called exclusively locked ;) > > - Extending something like LWLockHeldByMe to also return the current > > lockmode doesn't sound hard > > - we seem to be using #ifdef NOT_YET for such cases > > After spending some time trying to make this work usefully, I observed > that it's pointless, at least if we apply it only in > HeapTupleSatisfiesUpdate, because the IS_MULTI bit is going to be > removed by compute_new_xmax_infomask if the multi is gone. Something > like this would be useful if we could do it in other places; say when > we're merely scanning page without the specific intention of modifying > that particular tuple. Maybe in heap_prune_page, for example. ISTM we > can see that as an optimization we can add later. heap_prune_page sounds like a good fit, yes. One other place would be when following hot chains, but the locking would be far more critical in that path, so its less likely to be beneficial. Pushing it off sounds good to me. > Some of your other observations have been fixed by commits that I have > pushed to my github tree. A short repetition of the previous pgbench run of many SELECT FOR SHARE's: 10s test: master: 22673 24637 23874 25527 fklocks: 24835 24601 24606 24868 60s test: master: 32609 33087 fklocks: 33350 33359 Very nice! Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-26T19:45:55Z
Here's version 24. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks
Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-27T15:07:34Z
Alvaro Herrera wrote: > Here's version 24. Old review emails still contains some items that didn't lead to any changes. I tried to keep close track of those. To that list I add a couple of things of my own. Here they are, for those following along. I welcome suggestions. - Fix the multixact cache in multixact.c -- see mXactCacheEnt. - ResetHintBitMulti() was removed; need to remove old XMAX_IS_MULTI somewhere; perhaps during HOT page pruning? - EvalPlanQual and ExecLockRows maybe need a different overall locking strategy. Right now follow_updates=false is used to avoid deadlocks. - Ensure multixact.c behaves sanely on wraparound. - Is the case of a a non-key-update followed by a key-update actually handled when doing a heap_lock_tuple with mode = LockTupleKeyShare and follow_updates = false? I don't really see how, so it seems to deserve at least a comment. - if oldestMultiXactId + db is set and then that database is dropped we seem to have a problem because MultiXactAdvanceOldest won't overwrite those values. Should probably use SetMultiXactIdLimit directly. - what stop multixacts only being filled out (i.e RecordNewMultiXact()) *after* the XLogInsert() *and* after a MultiXactGetCheckptMulti()? Afaics MultiXactGenLock is not hold in CreateMultiXactId(). If we crash in that moment we loose the multixact data which now means potential data loss... - multixact member group data crossing 512 sector boundaries makes me uneasy (as its 5 bytes). I don't really see a scenario where its dangerous, but ... Does anybody see a problem here? - there are quite some places that do multiStopLimit = multiWrapLimit - 100; if (multiStopLimit < FirstMultiXactId) multiStopLimit -= FirstMultiXactId; perhaps MultiXactIdAdvance and MultiXactIdRetreat macros are in order? - not really padding, MultiXactStatus is 4bytes... /* * XXX Note: there's a lot of padding space in MultiXactMember. We could * find a more compact representation of this Xlog record -- perhaps all the * status flags in one XLogRecData, then all the xids in another one? Not * clear that it's worth the trouble though. */ -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
-
Re: foreign key locks - EvalPlanQual interactions
Andres Freund <andres@2ndquadrant.com> — 2012-11-29T19:25:59Z
On 2012-11-27 12:07:34 -0300, Alvaro Herrera wrote: > Alvaro Herrera wrote: > > Here's version 24. > > Old review emails still contains some items that didn't lead to any > changes. I tried to keep close track of those. To that list I add a > couple of things of my own. Here they are, for those following along. > I welcome suggestions. > > - EvalPlanQual and ExecLockRows maybe need a different overall locking > strategy. Right now follow_updates=false is used to avoid deadlocks. I think this really might need some work. Afaics the EPQ code now needs to actually determine what locklevel needs to be passed to EvalPlanQualFetch via EvalPlanQual otherwise some of the locking issues that triggered all this remain. That sucks a bit from a modularity perspective, but I don't see another way. Greetings, Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services