Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Mon, Feb 22, 2016 at 7:59 PM, Tom Lane wrote: >> No, you don't. I've spent a good deal of time thinking about that problem. >> [ much snipped ] >> Unless I'm missing something, though, this is a fairly obscure >> problem. Early release of catalog locks is desirable, and locks on >> scanned tables should be the same locks (or weaker) than already held >> by the master. Other cases are rare. I think. It would be good to >> know if you think otherwise. > > After further thought, what I think about this is that it's safe so long > as parallel workers are strictly read-only. Given that, early lock > release after user table access is okay for the same reasons it's okay > after catalog accesses. However, this is one of the big problems that > we'd have to have a solution for before we ever consider allowing > read-write parallelism. Actually, I don't quite see what read-only vs. read-write queries has to do with this particular issue. We retain relation locks on target relations until commit, regardless of whether those locks are AccessShareLock, RowShareLock, or RowExclusiveLock. As far as I understand it, this isn't because anything would fail horribly if we released those locks at end of query, but rather because we think that releasing those locks early might result in unpleasant surprises for client applications. I'm actually not really convinced that's true: I will grant that it might be surprising to run the same query twice in the same transaction and get different tuple descriptors, but it might also be surprising to get different rows, which READ COMMITTED allows anyway. And I've met a few users who were pretty surprised to find out that they couldn't do DDL on table A and the blocking session mentioned table A nowhere in the currently-executing query. The main issues with allowing read-write parallelism that I know of off-hand are: * Updates or deletes might create new combo CIDs. In order to handle that, we'd need to store the combo CID mapping in some sort of DSM-based data structure which could expand as new combo CIDs were generated. * Relation extension locks, and a few other types of heavyweight locks, are used for mutual exclusion of operations that would need to be serialized even among cooperating backends. So group locking would need to be enhanced to handle those cases differently, or some other solution would need to be found. (I've done some more detailed analysis here about possible solutions most of which has been posted to -hackers in various emails at one time or another; I'll refrain from diving into all the details in this missive.) But those are separate from the question of whether parallel workers need to transfer any heavyweight locks they accumulate on non-scanned tables back to the leader. > So what distresses me about the current situation is that this is a large > stumbling block that I don't see documented anywhere. It'd be good if you > transcribed some of this conversation into README.parallel. > > (BTW, I don't believe your statement that transferring locks back to the > master would be deadlock-prone. If the lock system treats locks held by > a lock group as effectively all belonging to one entity, then granting a > lock identical to one already held by another group member should never > fail. I concur that it might be expensive performance-wise, though it > hardly seems like this would be a dominant factor compared to all the > other costs of setting up and tearing down parallel workers.) I don't mean that the heavyweight lock acquisition itself would fail; I agree with your analysis on that. I mean that you'd have to design the protocol for the leader and the worker to communicate very carefully in order for it not to get stuck. Right now, the leader initializes the DSM at startup before any workers are running with all the data the workers will need, and after that data flows strictly from workers to leader. So the workers could send control messages indicating heavyweight locks that they held to the leader, and that would be fine. Then the leader would need to read those messages and do something with them, after which it would need to tell the workers that they could now exit. You'd need to make sure there was no situation in which that handshake couldn't get stuck, for example because the leader was waiting for a tuple from the worker while the worker was waiting for a lock-acquisition-confirmation from the leader. That particular thing is probably not an issue but hopefully it illustrates the sort of hazard I'm concerned about. -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> ... However, this is one of the big problems that >> we'd have to have a solution for before we ever consider allowing >> read-write parallelism. > Having such a blocker for read-write parallelism would be unfortunate, > though perhaps there isn't much help for it, and having read-only query > parallelism is certainly far better than nothing. IIUC, this is not the only large problem standing between us and read-write parallelism, and probably not even the biggest one. So I'm basically just asking that it gets documented while it's fresh in mind, rather than leaving it for some future hackers to rediscover the hard way. (Wouldn't be bad to doc the other known stumbling blocks, too.) 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Robert Haas writes: > > On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane wrote: > >> I just had a rather disturbing thought to the effect that this entire > >> design --- ie, parallel workers taking out locks for themselves --- is > >> fundamentally flawed. As far as I can tell from README.parallel, > >> parallel workers are supposed to exit (and, presumably, release their > >> locks) before the leader's transaction commits. Releasing locks before > >> commit is wrong. Do I need to rehearse why? > > > No, you don't. I've spent a good deal of time thinking about that problem. > > [ much snipped ] > > Unless I'm missing something, though, this is a fairly obscure > > problem. Early release of catalog locks is desirable, and locks on > > scanned tables should be the same locks (or weaker) than already held > > by the master. Other cases are rare. I think. It would be good to > > know if you think otherwise. > > After further thought, what I think about this is that it's safe so long > as parallel workers are strictly read-only. Given that, early lock > release after user table access is okay for the same reasons it's okay > after catalog accesses. However, this is one of the big problems that > we'd have to have a solution for before we ever consider allowing > read-write parallelism. Having such a blocker for read-write parallelism would be unfortunate, though perhaps there isn't much help for it, and having read-only query parallelism is certainly far better than nothing. > So what distresses me about the current situation is that this is a large > stumbling block that I don't see documented anywhere. It'd be good if you > transcribed some of this conversation into README.parallel. Agreed. > (BTW, I don't believe your statement that transferring locks back to the > master would be deadlock-prone. If the lock system treats locks held by > a lock group as effectively all belonging to one entity, then granting a > lock identical to one already held by another group member should never > fail. I concur that it might be expensive performance-wise, though it > hardly seems like this would be a dominant factor compared to all the > other costs of setting up and tearing down parallel workers.) This is only when a parallel worker is finished, no? Isn't there already some indication of when a parallel worker is done that the master handles, where it could also check the shared lock table and see if any locks were transferred to it on worker exit? Only following this thread from afar, so take my suggestions with a grain of salt. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: > On Mon, Feb 22, 2016 at 2:56 AM, Tom Lane wrote: > !held by the indicated process. False indicates that this process is > !currently waiting to acquire this lock, which implies that at > least one other > !process is holding a conflicting lock mode on the same lockable object. > I know you're just updating existing language here, but this is false. > It only implies that one other process is holding *or waiting for* a > conflicting lock mode on the same lockable object. True. I had considered whether to fix that point as well, and decided that it might just be overcomplicating matters. But since you complain, I'll add "or waiting for". It also occurred to me last night that pg_blocking_pids() needs a disclaimer similar to the existing one for pg_locks about how using it a lot could put a performance drag on the system. Other than adjusting those points, I think this is ready to go, and will commit later today if I hear no objections. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: > On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane wrote: >> I just had a rather disturbing thought to the effect that this entire >> design --- ie, parallel workers taking out locks for themselves --- is >> fundamentally flawed. As far as I can tell from README.parallel, >> parallel workers are supposed to exit (and, presumably, release their >> locks) before the leader's transaction commits. Releasing locks before >> commit is wrong. Do I need to rehearse why? > No, you don't. I've spent a good deal of time thinking about that problem. > [ much snipped ] > Unless I'm missing something, though, this is a fairly obscure > problem. Early release of catalog locks is desirable, and locks on > scanned tables should be the same locks (or weaker) than already held > by the master. Other cases are rare. I think. It would be good to > know if you think otherwise. After further thought, what I think about this is that it's safe so long as parallel workers are strictly read-only. Given that, early lock release after user table access is okay for the same reasons it's okay after catalog accesses. However, this is one of the big problems that we'd have to have a solution for before we ever consider allowing read-write parallelism. So what distresses me about the current situation is that this is a large stumbling block that I don't see documented anywhere. It'd be good if you transcribed some of this conversation into README.parallel. (BTW, I don't believe your statement that transferring locks back to the master would be deadlock-prone. If the lock system treats locks held by a lock group as effectively all belonging to one entity, then granting a lock identical to one already held by another group member should never fail. I concur that it might be expensive performance-wise, though it hardly seems like this would be a dominant factor compared to all the other costs of setting up and tearing down parallel workers.) 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Mon, Feb 22, 2016 at 2:56 AM, Tom Lane wrote: > I wrote: >> Robert Haas writes: >>> As for the patch itself, I'm having trouble grokking what it's trying >>> to do. I think it might be worth having a comment defining precisely >>> what we mean by "A blocks B". I would define "A blocks B" in general >>> as either A holds a lock which conflicts with one sought by B >>> (hard-blocked) or A awaits a lock which conflicts with one sought by B >>> and precedes it in the wait queue (soft-blocked). > >> Yes, that is exactly what I implemented ... and it's something you can't >> find out from pg_locks. I'm not sure how that view could be made to >> expose wait-queue ordering. > > Here's an updated version of this patch, now with user-facing docs. > > I decided that "pg_blocking_pids()" is a better function name than > "pg_blocker_pids()". The code's otherwise the same, although I > revisited some of the comments. > > I also changed quite a few references to "transaction" into "process" > in the discussion of pg_locks. The previous choice to conflate > processes with transactions was never terribly wise in my view, and > it's certainly completely broken by parallel query. !held by the indicated process. False indicates that this process is !currently waiting to acquire this lock, which implies that at least one other !process is holding a conflicting lock mode on the same lockable object. I know you're just updating existing language here, but this is false. It only implies that one other process is holding *or waiting for* a conflicting lock mode on the same lockable object. Other than that, I think the documentation changes look good. -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane wrote: > I just had a rather disturbing thought to the effect that this entire > design --- ie, parallel workers taking out locks for themselves --- is > fundamentally flawed. As far as I can tell from README.parallel, > parallel workers are supposed to exit (and, presumably, release their > locks) before the leader's transaction commits. Releasing locks before > commit is wrong. Do I need to rehearse why? No, you don't. I've spent a good deal of time thinking about that problem. In typical cases, workers are going to be acquiring either catalog locks (which are released before commit) or locks on relations which the leader has already locked (in which case the leader will still hold the lock - or possibly a stronger one - even after the worker releases that lock). Suppose, however, that you write a function which goes and queries some other table not involved in the query, and therefore acquires a lock on it. If you mark that function PARALLEL SAFE and it runs only in the worker and not in in the leader, then you could end up with a parallel query that releases the lock before commit where a non-parallel version of that query would have held the lock until transaction commit. Of course, one answer to this problem is - if the early lock release is apt to be a problem for you - don't mark such functions PARALLEL SAFE. I've thought about engineering a better solution. Two possible designs come to mind. First, we could have the worker send to the leader a list of locks that it holds at the end of its work, and the leader could acquire all of those before confirming to the worker that it is OK to terminate. That has some noteworthy disadvantages, like being prone to deadlock and requiring workers to stick around potentially quite a bit longer than they do at present, thus limiting the ability of other processes to access parallel query. Second, we could have the workers reassign all of their locks to the leader in the lock table (unless the leader already holds that lock). The problem with that is that then the leader is in the weird situation of having locks in the shared lock table that it doesn't know anything about - they don't appear in it's local lock table. How does the leader decide which resource owner they go with? Unless I'm missing something, though, this is a fairly obscure problem. Early release of catalog locks is desirable, and locks on scanned tables should be the same locks (or weaker) than already held by the master. Other cases are rare. I think. It would be good to know if you think otherwise. -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
I wrote: > Robert Haas writes: >> As for the patch itself, I'm having trouble grokking what it's trying >> to do. I think it might be worth having a comment defining precisely >> what we mean by "A blocks B". I would define "A blocks B" in general >> as either A holds a lock which conflicts with one sought by B >> (hard-blocked) or A awaits a lock which conflicts with one sought by B >> and precedes it in the wait queue (soft-blocked). > Yes, that is exactly what I implemented ... and it's something you can't > find out from pg_locks. I'm not sure how that view could be made to > expose wait-queue ordering. Here's an updated version of this patch, now with user-facing docs. I decided that "pg_blocking_pids()" is a better function name than "pg_blocker_pids()". The code's otherwise the same, although I revisited some of the comments. I also changed quite a few references to "transaction" into "process" in the discussion of pg_locks. The previous choice to conflate processes with transactions was never terribly wise in my view, and it's certainly completely broken by parallel query. regards, tom lane diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index d77e999..d3270e4 100644 *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** *** 8015,8030 The view pg_locks provides access to !information about the locks held by open transactions within the database server. See for more discussion of locking. pg_locks contains one row per active lockable !object, requested lock mode, and relevant transaction. Thus, the same lockable object might !appear many times, if multiple transactions are holding or waiting for locks on it. However, an object that currently has no locks on it will not appear at all. --- 8015,8030 The view pg_locks provides access to !information about the locks held by active processes within the database server. See for more discussion of locking. pg_locks contains one row per active lockable !object, requested lock mode, and relevant process. Thus, the same lockable object might !appear many times, if multiple processs are holding or waiting for locks on it. However, an object that currently has no locks on it will not appear at all. *** *** 8200,8210 granted is true in a row representing a lock !held by the indicated transaction. False indicates that this transaction is !currently waiting to acquire this lock, which implies that some other !transaction is holding a conflicting lock mode on the same lockable object. !The waiting transaction will sleep until the other lock is released (or a !deadlock situation is detected). A single transaction can be waiting to acquire at most one lock at a time. --- 8200,8210 granted is true in a row representing a lock !held by the indicated process. False indicates that this process is !currently waiting to acquire this lock, which implies that at least one other !process is holding a conflicting lock mode on the same lockable object. !The waiting process will sleep until the other lock is released (or a !deadlock situation is detected). A single process can be waiting to acquire at most one lock at a time. *** *** 8224,8230 Although tuples are a lockable type of object, information about row-level locks is stored on disk, not in memory, and therefore row-level locks normally do not appear in this view. !If a transaction is waiting for a row-level lock, it will usually appear in the view as waiting for the permanent transaction ID of the current holder of that row lock. --- 8224,8230 Although tuples are a lockable type of object, information about row-level locks is stored on disk, not in memory, and therefore row-level locks normally do not appear in this view. !If a process is waiting for a row-level lock, it will usually appear in the view as waiting for the permanent transaction ID of the current holder of that row lock. *** SELECT * FROM pg_locks pl LEFT JOIN pg_p *** 8281,8286 --- 8281,8300 +While it is possible to obtain information about which processes block +which other processes by joining pg_locks against +itself, this is very difficult to get right in detail. Such a query would +have to encode knowledge about which lock modes conflict with which +others. Worse, the pg_locks view does not expose +information about which processes are ahead of which others in lock wait +queues, nor information about which processes are parallel workers running +on behalf of which other client sessions. It is better to use +the pg_blocking_pids() fu
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
I just had a rather disturbing thought to the effect that this entire design --- ie, parallel workers taking out locks for themselves --- is fundamentally flawed. As far as I can tell from README.parallel, parallel workers are supposed to exit (and, presumably, release their locks) before the leader's transaction commits. Releasing locks before commit is wrong. Do I need to rehearse why? 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: > On Tue, Feb 16, 2016 at 2:59 AM, Tom Lane wrote: >> Not to be neglected also is that (I believe) this gives the right answer, >> whereas isolationtester's existing query is currently completely broken by >> parallel queries, and it doesn't understand non-conflicting lock modes >> either. (It did, at least partially, before commit 38f8bdcac4982215; >> I am not sure that taking out the mode checks was a good idea. But >> putting them back would make the query slower yet.) > The reason I took that out is because it breaks the deadlock-soft > test. It's possible to have a situation where no granted lock > conflicts with an awaited lock. If that happens, the old query > wrongly concluded that the waiting process was not in fact waiting. > (Consider A hold AccessShareLock, B awaits AccessExclusiveLock, C now > requests AccessShareLock and *waits*.) Ah, well, with this patch the deadlock-soft test still passes. > As for the patch itself, I'm having trouble grokking what it's trying > to do. I think it might be worth having a comment defining precisely > what we mean by "A blocks B". I would define "A blocks B" in general > as either A holds a lock which conflicts with one sought by B > (hard-blocked) or A awaits a lock which conflicts with one sought by B > and precedes it in the wait queue (soft-blocked). Yes, that is exactly what I implemented ... and it's something you can't find out from pg_locks. I'm not sure how that view could be made to expose wait-queue ordering. > For parallel queries, there's a further relevant distinction when we > say "A blocks B". We might mean either that (1) process B cannot > resume execution until the lock conflict is resolved or (2) the group > leader for process B cannot complete the current parallel operation > until the lock conflict is resolved. The definition I used in this patch is "some member of A's lock group blocks some member of B's lock group", because that corresponds directly to whether A is preventing B's query from completing, which is what isolationtester wants to know --- and, I would argue, it's generally what any client would want to know. 99.9% of clients would just as soon not be aware of parallel workers lurking underneath the pg_backend_pid() values that they see for their sessions. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Tue, Feb 16, 2016 at 2:59 AM, Tom Lane wrote: > Andres Freund writes: >> I wonder if we shouldn't just expose a 'which pid is process X waiting >> for' API, implemented serverside. That's generally really useful, and >> looks like it's actually going to be less complicated than that >> query... And it's surely going to be faster. > > Attached is a draft patch for a new function that reports the set of PIDs > directly blocking a given PID (that is, holding or awaiting conflicting > locks on the lockable object it's waiting for). > > I replaced isolationtester's pg_locks query with this, and found that > it's about 9X faster in a normal build, and 3X faster with > CLOBBER_CACHE_ALWAYS turned on. That would give us some nice headroom > for the isolation tests with CLOBBER_CACHE_ALWAYS animals. (Note that > in view of > http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=markhor&dt=2016-02-14%2007%3A38%3A37 > we still need to do *something* about the speed of the new deadlock-hard > test; this patch could avoid the need to dumb down or slow down that test > even further.) > > Not to be neglected also is that (I believe) this gives the right answer, > whereas isolationtester's existing query is currently completely broken by > parallel queries, and it doesn't understand non-conflicting lock modes > either. (It did, at least partially, before commit 38f8bdcac4982215; > I am not sure that taking out the mode checks was a good idea. But > putting them back would make the query slower yet.) The reason I took that out is because it breaks the deadlock-soft test. It's possible to have a situation where no granted lock conflicts with an awaited lock. If that happens, the old query wrongly concluded that the waiting process was not in fact waiting. (Consider A hold AccessShareLock, B awaits AccessExclusiveLock, C now requests AccessShareLock and *waits*.) As for the patch itself, I'm having trouble grokking what it's trying to do. I think it might be worth having a comment defining precisely what we mean by "A blocks B". I would define "A blocks B" in general as either A holds a lock which conflicts with one sought by B (hard-blocked) or A awaits a lock which conflicts with one sought by B and precedes it in the wait queue (soft-blocked). I have wondered before if we shouldn't modify pg_locks to expose the wait-queue ordering; without that, you can't reliably determine in general whether A soft-blocks B, which means every view anyone has ever written over pg_locks that purports to say who blocks who is necessarily buggy. For parallel queries, there's a further relevant distinction when we say "A blocks B". We might mean either that (1) process B cannot resume execution until the lock conflict is resolved or (2) the group leader for process B cannot complete the current parallel operation until the lock conflict is resolved. If you're trying to figure out why one particular member of a parallel group is stuck, you want to answer question #1. If you're trying to figure out what all the things that need to get out of the way to finish the query, you want to answer question #2. I think this function is aiming to answer question #2, not question #1, but I'm less clear on the reason behind that definitional choice. -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Andres Freund writes: > I wonder if we shouldn't just expose a 'which pid is process X waiting > for' API, implemented serverside. That's generally really useful, and > looks like it's actually going to be less complicated than that > query... And it's surely going to be faster. Attached is a draft patch for a new function that reports the set of PIDs directly blocking a given PID (that is, holding or awaiting conflicting locks on the lockable object it's waiting for). I replaced isolationtester's pg_locks query with this, and found that it's about 9X faster in a normal build, and 3X faster with CLOBBER_CACHE_ALWAYS turned on. That would give us some nice headroom for the isolation tests with CLOBBER_CACHE_ALWAYS animals. (Note that in view of http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=markhor&dt=2016-02-14%2007%3A38%3A37 we still need to do *something* about the speed of the new deadlock-hard test; this patch could avoid the need to dumb down or slow down that test even further.) Not to be neglected also is that (I believe) this gives the right answer, whereas isolationtester's existing query is currently completely broken by parallel queries, and it doesn't understand non-conflicting lock modes either. (It did, at least partially, before commit 38f8bdcac4982215; I am not sure that taking out the mode checks was a good idea. But putting them back would make the query slower yet.) This is WIP, in part because I've written no user docs for the new function, and in part because I think people might want to bikeshed the API. What is here is: "pg_blocker_pids(integer) returns integer[]" returns a possibly-empty array of the PIDs of backend processes that block the backend with specified PID. You get an empty array, not an error, if the argument isn't a valid backend PID or that backend isn't waiting. In parallel query situations, the output includes PIDs that are blocking any PID in the given process's lock group, and what is reported is always the PID of the lock group leader for whichever process is kdoing the blocking. Also, in parallel query situations, the same PID might appear multiple times in the output; it didn't seem worth trying to eliminate duplicates. Reasonable API change requests might include returning a rowset rather than an array and returning more data per lock conflict. I've not bothered with either thing here because isolationtester doesn't care and it would make the query somewhat slower for isolationtester's usage (though, probably, not enough slower to really matter). It should also be noted that I've not really tested the parallel query aspects of this, because I'm not sure how to create a conflicting lock request in a parallel worker. However, if I'm not still misunderstanding the new semantics of the lock data structures, that aspect of it seems unlikely to be wrong. Comments? regards, tom lane diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 91218d0..97e8962 100644 *** a/src/backend/storage/ipc/procarray.c --- b/src/backend/storage/ipc/procarray.c *** HaveVirtualXIDsDelayingChkpt(VirtualTran *** 2313,2318 --- 2313,2341 PGPROC * BackendPidGetProc(int pid) { + PGPROC *result; + + if (pid == 0)/* never match dummy PGPROCs */ + return NULL; + + LWLockAcquire(ProcArrayLock, LW_SHARED); + + result = BackendPidGetProcWithLock(pid); + + LWLockRelease(ProcArrayLock); + + return result; + } + + /* + * BackendPidGetProcWithLock -- get a backend's PGPROC given its PID + * + * Same as above, except caller must be holding ProcArrayLock. The found + * entry, if any, can be assumed to be valid as long as the lock remains held. + */ + PGPROC * + BackendPidGetProcWithLock(int pid) + { PGPROC *result = NULL; ProcArrayStruct *arrayP = procArray; int index; *** BackendPidGetProc(int pid) *** 2320,2327 if (pid == 0)/* never match dummy PGPROCs */ return NULL; - LWLockAcquire(ProcArrayLock, LW_SHARED); - for (index = 0; index < arrayP->numProcs; index++) { PGPROC *proc = &allProcs[arrayP->pgprocnos[index]]; --- 2343,2348 *** BackendPidGetProc(int pid) *** 2333,2340 } } - LWLockRelease(ProcArrayLock); - return result; } --- 2354,2359 diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index fef59a2..fb32769 100644 *** a/src/backend/storage/lmgr/lock.c --- b/src/backend/storage/lmgr/lock.c *** *** 21,27 * * Interface: * ! * InitLocks(), GetLocksMethodTable(), * LockAcquire(), LockRelease(), LockReleaseAll(), * LockCheckConflicts(), GrantLock() * --- 21,27 * * Interface: * ! * InitLocks(), GetLocksMethodTable(), GetLockTagsMethodTable(), * LockAcquire(), LockRelease(), LockReleaseAll(), * LockCheckConflicts(), GrantLock() * *** *** 41,46 --- 41,47
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Greg Stark writes: > The tests worked fine on faster build animals, right? And the clobber > animals are much much slower So it seems perfectly sensible that their > deadlock timeout would just have to be much much higher to have the same > behaviour. I see nothing wrong in just setting deadlock timeout to a minute > or more on these machines. We don't have a way to make the isolation tests change behavior depending on how the backend is compiled. So the only actually available fix is to make that test take "a minute or more" for *everybody*. Aside from that, it's just disturbing that these tests aren't deterministic regardless of machine speed. We don't seem to have a way around that right now, but I wish we did. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: > On Fri, Feb 12, 2016 at 4:59 PM, Tom Lane wrote: >> The fundamental problem I fear is that isolationtester is designed around >> the assumption that only its own actions (query issuances) change the >> state of the database. Trying to use it to test deadlock detection is >> problematic because deadlock-breaking changes the DB state asynchronously. > Maybe we should introduce a way to declare whether a step is expected > to wait or not. I thought about doing that, and the only reason I > didn't is because I couldn't figure out a reasonable syntax. But, in > many respects, that would actually be better than the current system > of having isolationtester try to figure it out itself. Meh. I'm not sure that would actually help anything. The problem is not so much with figuring out whether a step blocks, as knowing when it's expected to complete. And for sure I don't want to annotate the spec files to the point of saying "this action should cause these other steps to complete". Actually ... the thing we've been fighting over the past couple days is having to make step completion orders deterministic when in principle they aren't and don't need to be. Maybe the solution is something like the core tests' variant expected files, ugly though those are. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: > On Fri, Feb 12, 2016 at 6:22 PM, Andres Freund wrote: >> I wonder if we shouldn't just expose a 'which pid is process X waiting >> for' API, implemented serverside. That's generally really useful, and >> looks like it's actually going to be less complicated than that >> query... And it's surely going to be faster. > If PID 12000 and PID 13000 hold AccessShareLock on relation foo, and > PID 14000 awaits AccessExclusiveLock on that relation, what does the > function return when 14000 is passed as an argument? Yeah. In general, it's not that easy to say that A is waiting specifically on B --- there may be multiple blockers and/or multiple ways it could get released. isolationtester's query is not really correct IMO. It's okay as long as nothing else besides autovacuum is taking locks, but I wouldn't want to try to make it work in a general purpose environment. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Fri, Feb 12, 2016 at 6:22 PM, Andres Freund wrote: > On 2016-02-12 13:16:54 -0500, Tom Lane wrote: >> Investigation showed that there are a couple of reasons. One, >> isolationtester's is-it-waiting query takes an insane amount of >> time under CLOBBER_CACHE_ALWAYS --- over half a second on my >> reasonably new server. > > I wonder if we shouldn't just expose a 'which pid is process X waiting > for' API, implemented serverside. That's generally really useful, and > looks like it's actually going to be less complicated than that > query... And it's surely going to be faster. If PID 12000 and PID 13000 hold AccessShareLock on relation foo, and PID 14000 awaits AccessExclusiveLock on that relation, what does the function return when 14000 is passed as an argument? -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On 2016-02-12 13:16:54 -0500, Tom Lane wrote: > Investigation showed that there are a couple of reasons. One, > isolationtester's is-it-waiting query takes an insane amount of > time under CLOBBER_CACHE_ALWAYS --- over half a second on my > reasonably new server. I wonder if we shouldn't just expose a 'which pid is process X waiting for' API, implemented serverside. That's generally really useful, and looks like it's actually going to be less complicated than that query... And it's surely going to be faster. Andres -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Fri, Feb 12, 2016 at 4:59 PM, Tom Lane wrote: > I wrote: >> Instead, what I propose we do about this is to change isolationtester >> so that once it's decided that a given step is blocked, it no longer >> issues the is-it-waiting query for that step; it just assumes that the >> step should be treated as blocked. So all we need do for "backlogged" >> steps is check PQisBusy/PQconsumeInput. That both greatly reduces the >> number of is-it-waiting queries that are needed and avoids any flappy >> behavior of the answer. > > Hmm, that seemed to work fine here, but the buildfarm is not very happy > with it, and on reflection I guess it's just moving the indeterminacy > somewhere else. If we check for completion of a given step, and don't > wait till it's either completed or known blocked, then we have a race > condition that can change the order in which completions are reported. > > The fundamental problem I fear is that isolationtester is designed around > the assumption that only its own actions (query issuances) change the > state of the database. Trying to use it to test deadlock detection is > problematic because deadlock-breaking changes the DB state asynchronously. > > I think what we have to do is revert that change and dumb down > deadlock-hard until it produces stable results even on the CLOBBER > critters. One thing that'd help is reducing the number of processes > involved --- AFAICS, testing an 8-way deadlock is not really any more > interesting than testing, say, 4-way, and that would halve the amount > of time isolationtester spends figuring out the state. Maybe we should introduce a way to declare whether a step is expected to wait or not. I thought about doing that, and the only reason I didn't is because I couldn't figure out a reasonable syntax. But, in many respects, that would actually be better than the current system of having isolationtester try to figure it out itself. -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
I wrote: > Instead, what I propose we do about this is to change isolationtester > so that once it's decided that a given step is blocked, it no longer > issues the is-it-waiting query for that step; it just assumes that the > step should be treated as blocked. So all we need do for "backlogged" > steps is check PQisBusy/PQconsumeInput. That both greatly reduces the > number of is-it-waiting queries that are needed and avoids any flappy > behavior of the answer. Hmm, that seemed to work fine here, but the buildfarm is not very happy with it, and on reflection I guess it's just moving the indeterminacy somewhere else. If we check for completion of a given step, and don't wait till it's either completed or known blocked, then we have a race condition that can change the order in which completions are reported. The fundamental problem I fear is that isolationtester is designed around the assumption that only its own actions (query issuances) change the state of the database. Trying to use it to test deadlock detection is problematic because deadlock-breaking changes the DB state asynchronously. I think what we have to do is revert that change and dumb down deadlock-hard until it produces stable results even on the CLOBBER critters. One thing that'd help is reducing the number of processes involved --- AFAICS, testing an 8-way deadlock is not really any more interesting than testing, say, 4-way, and that would halve the amount of time isolationtester spends figuring out the state. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: > On Thu, Feb 11, 2016 at 11:34 PM, Tom Lane wrote: >> The problem here is that when the deadlock detector kills s8's >> transaction, s7a8 is also left free to proceed, so there is a race >> condition as to which query completion will get back to >> isolationtester first. >> >> One grotty way to handle that would be something like >> >> -step "s7a8"{ LOCK TABLE a8; } >> +step "s7a8"{ LOCK TABLE a8; SELECT pg_sleep(5); } >> >> Or we could simplify the locking structure enough so that no other >> transactions are released by the deadlock failure. I do not know >> exactly what you had in mind to be testing here? > Basically just that the deadlock actually got detected. That may > sound a bit weak, but considering we had no test for it at all before > this... I tried fixing it as shown above, and was dismayed to find out that it didn't work, ie, there was still a difference between the regular output and the results with CLOBBER_CACHE_ALWAYS. In the latter case the printout makes it appear that s7a8 completed before s8a1, which is nonsensical. Investigation showed that there are a couple of reasons. One, isolationtester's is-it-waiting query takes an insane amount of time under CLOBBER_CACHE_ALWAYS --- over half a second on my reasonably new server. Probing the state of half a dozen blocked sessions thus takes a while. Second, once s8 has been booted out of its transaction, s7 is no longer "blocked" according to isolationtester's definition (it's doing the pg_sleep query instead). Therefore, when we're rechecking all the other blocked steps after detecting that s8 has become blocked, two things happen: enough time elapses for the deadlock detector to fire, and then when we get around to checking s7, we don't see it as blocked and therefore wait until it finishes. So s7a8 is reported first despite the pg_sleep, and would be no matter large a pg_sleep delay is used. We could possibly fix this by using a deadlock timeout even higher than 5 seconds, but that way madness lies. Instead, what I propose we do about this is to change isolationtester so that once it's decided that a given step is blocked, it no longer issues the is-it-waiting query for that step; it just assumes that the step should be treated as blocked. So all we need do for "backlogged" steps is check PQisBusy/PQconsumeInput. That both greatly reduces the number of is-it-waiting queries that are needed and avoids any flappy behavior of the answer. Comments? 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Thu, Feb 11, 2016 at 11:34 PM, Tom Lane wrote: > We're not out of the woods on this :-( ... jaguarundi, which is the first > of the CLOBBER_CACHE_ALWAYS animals to run these tests, didn't like them > at all. I think I fixed the deadlock-soft-2 failure, but its take on > deadlock-hard is: > > *** 17,25 > step s6a7: LOCK TABLE a7; > step s7a8: LOCK TABLE a8; > step s8a1: LOCK TABLE a1; > - step s8a1: <... completed> > step s7a8: <... completed> > ! error in steps s8a1 s7a8: ERROR: deadlock detected > step s8c: COMMIT; > step s7c: COMMIT; > step s6a7: <... completed> > --- 17,25 > step s6a7: LOCK TABLE a7; > step s7a8: LOCK TABLE a8; > step s8a1: LOCK TABLE a1; > step s7a8: <... completed> > ! step s8a1: <... completed> > ! ERROR: deadlock detected > step s8c: COMMIT; > step s7c: COMMIT; > step s6a7: <... completed> > > The problem here is that when the deadlock detector kills s8's > transaction, s7a8 is also left free to proceed, so there is a race > condition as to which query completion will get back to > isolationtester first. > > One grotty way to handle that would be something like > > -step "s7a8"{ LOCK TABLE a8; } > +step "s7a8"{ LOCK TABLE a8; SELECT pg_sleep(5); } > > Or we could simplify the locking structure enough so that no other > transactions are released by the deadlock failure. I do not know > exactly what you had in mind to be testing here? Basically just that the deadlock actually got detected. That may sound a bit weak, but considering we had no test for it at all before this... -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
We're not out of the woods on this :-( ... jaguarundi, which is the first of the CLOBBER_CACHE_ALWAYS animals to run these tests, didn't like them at all. I think I fixed the deadlock-soft-2 failure, but its take on deadlock-hard is: *** 17,25 step s6a7: LOCK TABLE a7; step s7a8: LOCK TABLE a8; step s8a1: LOCK TABLE a1; - step s8a1: <... completed> step s7a8: <... completed> ! error in steps s8a1 s7a8: ERROR: deadlock detected step s8c: COMMIT; step s7c: COMMIT; step s6a7: <... completed> --- 17,25 step s6a7: LOCK TABLE a7; step s7a8: LOCK TABLE a8; step s8a1: LOCK TABLE a1; step s7a8: <... completed> ! step s8a1: <... completed> ! ERROR: deadlock detected step s8c: COMMIT; step s7c: COMMIT; step s6a7: <... completed> The problem here is that when the deadlock detector kills s8's transaction, s7a8 is also left free to proceed, so there is a race condition as to which query completion will get back to isolationtester first. One grotty way to handle that would be something like -step "s7a8"{ LOCK TABLE a8; } +step "s7a8"{ LOCK TABLE a8; SELECT pg_sleep(5); } Or we could simplify the locking structure enough so that no other transactions are released by the deadlock failure. I do not know exactly what you had in mind to be testing here? 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
On Thu, Feb 11, 2016 at 12:04 PM, Tom Lane wrote: > I wrote: >> No, because the machines that are failing are showing a "" >> annotation that your reference output *doesn't* have. I think what is >> actually happening is that these machines are seeing the process as >> waiting and reporting it, whereas on your machine the backend detects >> the deadlock and completes the query (with an error) before >> isolationtester realizes that the process is waiting. > > I confirmed this theory by the expedient of changing the '10ms' setting > in the test script to 1ms (which worked) and 100ms (which did not, on > the same machine). > > I've committed an update that adjusts the timeouts to hopefully ensure > that isolationtester always sees the query as blocked before the deadlock > detector unblocks it; which seems like the behavior we want to test for, > anyway. Thanks. I really appreciate it. -- 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
I wrote: > No, because the machines that are failing are showing a "" > annotation that your reference output *doesn't* have. I think what is > actually happening is that these machines are seeing the process as > waiting and reporting it, whereas on your machine the backend detects > the deadlock and completes the query (with an error) before > isolationtester realizes that the process is waiting. I confirmed this theory by the expedient of changing the '10ms' setting in the test script to 1ms (which worked) and 100ms (which did not, on the same machine). I've committed an update that adjusts the timeouts to hopefully ensure that isolationtester always sees the query as blocked before the deadlock detector unblocks it; which seems like the behavior we want to test for, anyway. 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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.
Robert Haas writes: >> That would be great. Taking a look at what happened, I have a feeling >> this may be a race condition of some kind in the isolation tester. It >> seems to have failed to recognize that a1 started waiting, and that >> caused the "deadlock detected" message to reported differently. I'm >> not immediately sure what to do about that. > Yeah, so: try_complete_step() waits 10ms, and if it still hasn't > gotten any data back from the server, then it uses a separate query to > see whether the step in question is waiting on a lock. So what > must've happened here is that it took more than 10ms for the process > to show up as waiting in pg_stat_activity. No, because the machines that are failing are showing a "" annotation that your reference output *doesn't* have. I think what is actually happening is that these machines are seeing the process as waiting and reporting it, whereas on your machine the backend detects the deadlock and completes the query (with an error) before isolationtester realizes that the process is waiting. It would probably help if you didn't do this: setup { BEGIN; SET deadlock_timeout = '10ms'; } which pretty much guarantees that there is a race condition: you've set it so that the deadlock detector will run at approximately the same time when isolationtester will be probing the state. I'm surprised that it seemed to act consistently for you. I would suggest putting all the other sessions to deadlock_timeout of 100s and the one you want to fail to timeout of ~ 5s. That will mean that the "" output should show up pretty reliably even on overloaded buildfarm critters. 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