Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Robert Haas]

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.

[Tom Lane]

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.

[Stephen Frost]

* 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.

[Tom Lane]

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.

[Tom Lane]

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.

[Robert Haas]

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.

[Robert Haas]

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.

[Tom Lane]

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
+

Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Tom Lane]

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.

[Tom Lane]

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.

[Robert Haas]

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=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.

[Tom Lane]

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=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 = [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 

Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Tom Lane]

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]

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.

[Tom Lane]

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.

[Robert Haas]

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.

[Tom Lane]

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.

[Tom Lane]

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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Tom Lane]

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.

[Tom Lane]

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]

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