Horton

Thread

Commits

  1. Move isolationtester's is-blocked query into C code for speed.

  1. recent deadlock regression test failures

    Andres Freund <andres@anarazel.de> — 2017-04-07T16:57:49Z

    Hi,

There's two machines that recently report changes in deadlock detector
output:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hyrax&dt=2017-04-05%2018%3A58%3A04
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=friarbird&dt=2017-04-07%2004%3A20%3A01

both just failed twice in a row:
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=hyrax&br=HEAD
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=friarbird&br=HEAD
without previous errors of the same ilk.

I don't think any recent changes are supposed to affect deadlock
detector behaviour?

- Andres

  2. Re: recent deadlock regression test failures

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-04-07T17:10:50Z

    
On 04/07/2017 12:57 PM, Andres Freund wrote:
> Hi,
>
> There's two machines that recently report changes in deadlock detector
> output:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hyrax&dt=2017-04-05%2018%3A58%3A04
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=friarbird&dt=2017-04-07%2004%3A20%3A01
>
> both just failed twice in a row:
> https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=hyrax&br=HEAD
> https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=friarbird&br=HEAD
> without previous errors of the same ilk.
>
> I don't think any recent changes are supposed to affect deadlock
> detector behaviour?
>


Both these machines have CLOBBER_CACHE_ALWAYS set. And on both machines
recent changes have made the isolation tests run much much longer.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  3. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-07T17:28:11Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 04/07/2017 12:57 PM, Andres Freund wrote:
>> I don't think any recent changes are supposed to affect deadlock
>> detector behaviour?

> Both these machines have CLOBBER_CACHE_ALWAYS set. And on both machines
> recent changes have made the isolation tests run much much longer.

Ouch.  I see friarbird's run time for the isolation tests has gone from an
hour and change to over 5 hours in one fell swoop.  hyrax not much better.
Oddly, non-CCA animals don't seem to have changed much.

Eyeing recent patches, it seems like the culprit must be Kevin's
addition to isolationtester's wait query:

@@ -231,6 +231,14 @@ main(int argc, char **argv)
        appendPQExpBuffer(&wait_query, ",%s", backend_pids[i]);
    appendPQExpBufferStr(&wait_query, "}'::integer[]");
 
+   /* Also detect certain wait events. */
+   appendPQExpBufferStr(&wait_query,
+                        " OR EXISTS ("
+                        "  SELECT * "
+                        "  FROM pg_catalog.pg_stat_activity "
+                        "  WHERE pid = $1 "
+                        "  AND wait_event IN ('SafeSnapshot'))");
+
    res = PQprepare(conns[0], PREP_WAITING, wait_query.data, 0, NULL);
    if (PQresultStatus(res) != PGRES_COMMAND_OK)
    {

This seems a tad ill-considered.  We sweated a lot of blood not so long
ago to get the runtime of that query down, and this seems to have blown
it up again.  And done so for every isolation test case, not only the
ones where it could possibly matter.

			regards, tom lane

  4. Re: recent deadlock regression test failures

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-07T17:49:22Z

    On Fri, Apr 7, 2017 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> On 04/07/2017 12:57 PM, Andres Freund wrote:
>>> I don't think any recent changes are supposed to affect deadlock
>>> detector behaviour?
>
>> Both these machines have CLOBBER_CACHE_ALWAYS set. And on both machines
>> recent changes have made the isolation tests run much much longer.
>
> Ouch.  I see friarbird's run time for the isolation tests has gone from an
> hour and change to over 5 hours in one fell swoop.  hyrax not much better.
> Oddly, non-CCA animals don't seem to have changed much.
>
> Eyeing recent patches, it seems like the culprit must be Kevin's
> addition to isolationtester's wait query:

Ouch.  Without this we don't have regression test coverage for the
SERIALIZABLE READ ONLY DEFERRABLE code, but it's probably not worth
adding 4 hours to any tests, even if it only shows up with
CLOBBER_CACHE_ONLY.  I assume the consensus is that I should revert
it?

--
Kevin Grittner

  5. Re: recent deadlock regression test failures

    Andres Freund <andres@anarazel.de> — 2017-04-07T17:52:44Z

    On 2017-04-07 12:49:22 -0500, Kevin Grittner wrote:
> On Fri, Apr 7, 2017 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> >> On 04/07/2017 12:57 PM, Andres Freund wrote:
> >>> I don't think any recent changes are supposed to affect deadlock
> >>> detector behaviour?
> >
> >> Both these machines have CLOBBER_CACHE_ALWAYS set. And on both machines
> >> recent changes have made the isolation tests run much much longer.
> >
> > Ouch.  I see friarbird's run time for the isolation tests has gone from an
> > hour and change to over 5 hours in one fell swoop.  hyrax not much better.
> > Oddly, non-CCA animals don't seem to have changed much.
> >
> > Eyeing recent patches, it seems like the culprit must be Kevin's
> > addition to isolationtester's wait query:
> 
> Ouch.  Without this we don't have regression test coverage for the
> SERIALIZABLE READ ONLY DEFERRABLE code, but it's probably not worth
> adding 4 hours to any tests, even if it only shows up with
> CLOBBER_CACHE_ONLY.  I assume the consensus is that I should revert
> it?

I'd rather fix the issue, than remove the tests entirely.  Seems quite
possible to handle blocking on Safesnapshot in a similar manner as pg_blocking_pids?

Greetings,

Andres Freund

  6. Re: recent deadlock regression test failures

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-07T18:35:33Z

    On Fri, Apr 7, 2017 at 12:52 PM, Andres Freund <andres@anarazel.de> wrote:

> I'd rather fix the issue, than remove the tests entirely.  Seems quite
> possible to handle blocking on Safesnapshot in a similar manner as pg_blocking_pids?

I'll see what I can figure out.

-- 
Kevin Grittner

  7. Re: recent deadlock regression test failures

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-07T20:47:14Z

    On Sat, Apr 8, 2017 at 6:35 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Fri, Apr 7, 2017 at 12:52 PM, Andres Freund <andres@anarazel.de> wrote:
>
>> I'd rather fix the issue, than remove the tests entirely.  Seems quite
>> possible to handle blocking on Safesnapshot in a similar manner as pg_blocking_pids?
>
> I'll see what I can figure out.

Ouch.  These are the other ways I thought of to achieve this:

https://www.postgresql.org/message-id/CAEepm%3D1MR4Ug9YsLtOS4Q9KAU9aku0pZS4RhBN%3D0LY3pJ49Ksg%40mail.gmail.com

I'd be happy to write one of those, but it may take a day as I have
some other commitments.

-- 
Thomas Munro
http://www.enterprisedb.com

  8. Re: recent deadlock regression test failures

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-07T21:47:04Z

    On Fri, Apr 7, 2017 at 3:47 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Sat, Apr 8, 2017 at 6:35 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
>> On Fri, Apr 7, 2017 at 12:52 PM, Andres Freund <andres@anarazel.de> wrote:
>>
>>> I'd rather fix the issue, than remove the tests entirely.  Seems quite
>>> possible to handle blocking on Safesnapshot in a similar manner as pg_blocking_pids?
>>
>> I'll see what I can figure out.
>
> Ouch.  These are the other ways I thought of to achieve this:
>
> https://www.postgresql.org/message-id/CAEepm%3D1MR4Ug9YsLtOS4Q9KAU9aku0pZS4RhBN%3D0LY3pJ49Ksg%40mail.gmail.com
>
> I'd be happy to write one of those, but it may take a day as I have
> some other commitments.

Please give it a go.  I'm dealing with putting out fires with
customers while trying to make sure I have tested the predicate
locking GUCs patch sufficiently.  (I think it's ready to go, and has
passed all tests so far, but there are a few more I want to run.)
I'm not sure I can come up with a solution faster than you, given
that.  Since it is an improvement to performance for a test-only
environment on a feature that is already committed and not causing
problems for production environments, hopefully people will tolerate
a fix a day or two out.  If not, we'll have to revert and get it
into the first CF for v11.

--
Kevin Grittner

  9. Re: recent deadlock regression test failures

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-08T02:24:40Z

    On Sat, Apr 8, 2017 at 9:47 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Fri, Apr 7, 2017 at 3:47 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> On Sat, Apr 8, 2017 at 6:35 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
>>> On Fri, Apr 7, 2017 at 12:52 PM, Andres Freund <andres@anarazel.de> wrote:
>>>
>>>> I'd rather fix the issue, than remove the tests entirely.  Seems quite
>>>> possible to handle blocking on Safesnapshot in a similar manner as pg_blocking_pids?
>>>
>>> I'll see what I can figure out.
>>
>> Ouch.  These are the other ways I thought of to achieve this:
>>
>> https://www.postgresql.org/message-id/CAEepm%3D1MR4Ug9YsLtOS4Q9KAU9aku0pZS4RhBN%3D0LY3pJ49Ksg%40mail.gmail.com
>>
>> I'd be happy to write one of those, but it may take a day as I have
>> some other commitments.
>
> Please give it a go.  I'm dealing with putting out fires with
> customers while trying to make sure I have tested the predicate
> locking GUCs patch sufficiently.  (I think it's ready to go, and has
> passed all tests so far, but there are a few more I want to run.)
> I'm not sure I can come up with a solution faster than you, given
> that.  Since it is an improvement to performance for a test-only
> environment on a feature that is already committed and not causing
> problems for production environments, hopefully people will tolerate
> a fix a day or two out.  If not, we'll have to revert and get it
> into the first CF for v11.

Here is an attempt at option 2 from the menu I posted above.  Questions:

1.  Does anyone object to this extension of pg_blocking_pids()'s
remit?  If so, I could make it a separate function (that was option
3).

2.  Did I understand correctly that it is safe to scan the list of
SERIALIZABLEXACTs and access the possibleUnsafeConflicts list while
holding only SerializableXactHashLock, and that 'inLink' is the
correct link to be following?

-- 
Thomas Munro
http://www.enterprisedb.com

  10. Re: recent deadlock regression test failures

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-08T03:31:00Z

    On Fri, Apr 7, 2017 at 9:24 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

> 2.  Did I understand correctly that it is safe to scan the list of
> SERIALIZABLEXACTs and access the possibleUnsafeConflicts list while
> holding only SerializableXactHashLock,

Yes.

> and that 'inLink' is the correct link to be following?

If you're starting from the blocked (read-only) transaction (which
you are), inLink is the one to follow.

Note: It would be better form to use the SxactIsDeferrableWaiting()
macro than repeat the bit-testing code directly in your function.

--
Kevin Grittner

  11. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-08T04:22:04Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Here is an attempt at option 2 from the menu I posted above.  Questions:

> 1.  Does anyone object to this extension of pg_blocking_pids()'s
> remit?  If so, I could make it a separate function (that was option
> 3).

It seems an entirely principle-free change in the function's definition.

I'm not actually clear on why Kevin wanted this change in
isolationtester's wait behavior anyway, so maybe some clarification
on that would be a good idea.  But if we need it, I think probably
a dedicated function would be a good thing.  We want the wait-checking
query to be as trivial as possible at the SQL level, so whatever
semantic oddities it needs to have should be pushed into C code.

			regards, tom lane

  12. Re: recent deadlock regression test failures

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-08T13:11:08Z

    On Sat, Apr 8, 2017 at 4:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> Here is an attempt at option 2 from the menu I posted above.  Questions:
>
>> 1.  Does anyone object to this extension of pg_blocking_pids()'s
>> remit?  If so, I could make it a separate function (that was option
>> 3).
>
> It seems an entirely principle-free change in the function's definition.

Well... other backends can block a SERIALIZABLE DEFERRABLE
transaction, so it doesn't seem that unreasonable to expect that a
function named pg_blocking_pids(blocked_pid) described as "get array
of PIDs of sessions blocking specified backend PID" should be able to
tell you who they are.

You might say that pg_blocking_pid() is about locking only and not
arbitrary other kinds of waits, but safe snapshots are not completely
unrelated to locking if you tilt your head at the right angle:
GetSafeSnapshot waits for all transactions that might hold SIRead
locks that could affect this transaction's serializability to
complete.

But I can see that it's an odd case.  Minimal separate function
version attached.

> I'm not actually clear on why Kevin wanted this change in
> isolationtester's wait behavior anyway, so maybe some clarification
> on that would be a good idea.

I can't speak for Kevin but here's my argument as patch author:  One
of the purposes of the isolation tester is to test our transaction
isolation.  SERIALIZABLE DEFERRABLE is a special case of one of our
levels and should be tested.  Statement s3r in the new spec
read-only-anomaly-3.spec runs at that level and causes the backend to
wait for another backend.  Without any change to isolationtester it
would hang on that statement until timeout failure.  Therefore I
proposed that isolationtester should recognise this kind of waiting
and proceed to later steps that can unblock it, like so:

step s3r: SELECT id, balance FROM bank_account WHERE id IN ('X', 'Y')
ORDER BY id; <waiting ...>
step s2wx: UPDATE bank_account SET balance = -11 WHERE id = 'X';
step s2c: COMMIT;
step s3r: <... completed>

> But if we need it, I think probably
> a dedicated function would be a good thing.  We want the wait-checking
> query to be as trivial as possible at the SQL level, so whatever
> semantic oddities it needs to have should be pushed into C code.

Based on the above, here is a version that introduces a simple boolean
function pg_waiting_for_safe_snapshot(pid) and adds that the the
query.  This was my "option 1" upthread.

-- 
Thomas Munro
http://www.enterprisedb.com

  13. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-08T17:56:15Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Sat, Apr 8, 2017 at 4:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It seems an entirely principle-free change in the function's definition.

> You might say that pg_blocking_pid() is about locking only and not
> arbitrary other kinds of waits, but safe snapshots are not completely
> unrelated to locking if you tilt your head at the right angle:
> GetSafeSnapshot waits for all transactions that might hold SIRead
> locks that could affect this transaction's serializability to
> complete.

Well, the problem I'm having is that once we say that pg_blocking_pids()
is not just about heavyweight locks, it's not clear where we stop.
There might be many other cases where, if you dig around in the right data
structures, it'd be possible to say that process X is waiting for process
Y to do something.  How many of those will we expect pg_blocking_pids()
to cover?  And there certainly will be cases where X is waiting for Y but
we don't have any effective way of identifying that.  Is that a bug in
pg_blocking_pids()?

Admittedly, this is a somewhat academic objection; but I dislike taking a
function with a fairly clear, published spec and turning it into something
that does whatever's expedient for a single use-case.

> Based on the above, here is a version that introduces a simple boolean
> function pg_waiting_for_safe_snapshot(pid) and adds that the the
> query.  This was my "option 1" upthread.

Nah, this is not good either.  Yes, it's a fairly precise conversion
of what Kevin's patch did, but I think that patch is wrong even
without considering the performance angle.  If you look back at the
discussions in Feb 2016 that led to what we had, it turned out to be
important not just to be able to say that process X is blocked, but
that it is blocked by one of the other isolationtest sessions, and
not by some random third party (like autovacuum).  I do not know
whether there is, right now, any way for autovacuum to be the guilty
party for a SafeSnapshot wait --- but it does not seem like a good
plan to assume that there never will be.

So I think what we need is functionality very much like what you had
in the prior patch, ie identify the specific PIDs that are causing
process X to wait for a safe snapshot.  I'm just not happy with how
you packaged it.

Here's a sketch of what I think we ought to do:

1. Leave pg_blocking_pids() alone; it's a published API now.

2. Add GetSafeSnapshotBlockingPids() more or less as you had it
in the previous patch (+ Kevin's recommendations).  There might be
value in providing a SQL-level way to call that, but I'm not sure,
and it would be independent of fixing isolationtester anyway.

3. Invent a SQL function that is dedicated specifically to supporting
isolationtester and need not be documented at all; this gets us out
of the problem of whether it's okay to whack its semantics around
anytime somebody thinks of something else they want to test.
I'm imagining an API like

	  isolation_test_is_waiting_for(int, int[]) returns bool

so that isolationtester's query would reduce to something like

	SELECT pg_catalog.isolation_test_is_waiting_for($1, '{...}')

which would take even more cycles out of the parse/plan overhead for it
(which is basically what's killing the CCA animals).  Internally, this
function would call pg_blocking_pids and, if necessary,
GetSafeSnapshotBlockingPids, and would check for matches in its array
argument directly; it could probably do that significantly faster than the
general-purpose array && code.  So we'd have to expend a bit of backend C
code, but we'd have something that would be quite speedy and we could
customize in future without fear of breaking behavior that users are
depending on.

			regards, tom lane

  14. Re: recent deadlock regression test failures

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-09T00:26:15Z

    On Sat, Apr 8, 2017 at 12:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> On Sat, Apr 8, 2017 at 4:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

>> Based on the above, here is a version that introduces a simple boolean
>> function pg_waiting_for_safe_snapshot(pid) and adds that the the
>> query.  This was my "option 1" upthread.
>
> Nah, this is not good either.  Yes, it's a fairly precise conversion
> of what Kevin's patch did, but I think that patch is wrong even
> without considering the performance angle.  If you look back at the
> discussions in Feb 2016 that led to what we had, it turned out to be
> important not just to be able to say that process X is blocked, but
> that it is blocked by one of the other isolationtest sessions, and
> not by some random third party (like autovacuum).  I do not know
> whether there is, right now, any way for autovacuum to be the guilty
> party for a SafeSnapshot wait --- but it does not seem like a good
> plan to assume that there never will be.

It would make no sense to run autovacuum at the serializable
transaction isolation level, and only overlapping read-write
serializable transactions can block the attempt to acquire a safe
snapshot.

> So I think what we need is functionality very much like what you had
> in the prior patch, ie identify the specific PIDs that are causing
> process X to wait for a safe snapshot.  I'm just not happy with how
> you packaged it.
>
> Here's a sketch of what I think we ought to do:
>
> 1. Leave pg_blocking_pids() alone; it's a published API now.

Fair enough.

> 2. Add GetSafeSnapshotBlockingPids() more or less as you had it
> in the previous patch (+ Kevin's recommendations).  There might be
> value in providing a SQL-level way to call that, but I'm not sure,
> and it would be independent of fixing isolationtester anyway.

It seems entirely plausible that someone might want to know what is
holding up the start of a backup or large report which uses the READ
ONLY DEFERRABLE option, so I think there is a real use case for a
documented SQL function similar to pg_blocking_pids() to show the
pids of connections currently running transactions which are holding
things up.  Of course, they may not initially know whether it is
being blocked by heavyweight locks or concurrent serializable
read-write transactions, but it should not be a big deal to run two
separate functions.

Given the inability to run isolation tests to cover the deferrable
code, we used a variation on DBT-2 that could cause serialization
anomalies to generate a high concurrency saturation run using
serializable transactions, and started a SERIALIZABLE READ ONLY
DEFERRABLE transaction 1200 times competing with this load, timing
how long it took to start.  To quote the VLDB paper[1], "The median
latency was 1.98 seconds, with 90% of transactions able to obtain a
safe snapshot within 6 seconds, and all within 20 seconds. Given the
intended use (long-running transactions), we believe this delay is
reasonable."  That said, a single long-running serializable
read-write transaction could hold up the attempt indefinitely --
there is no maximum bound.  Hence the benefit of a SQL function to
find out what's happening.

> 3. Invent a SQL function that is dedicated specifically to supporting
> isolationtester and need not be documented at all; this gets us out
> of the problem of whether it's okay to whack its semantics around
> anytime somebody thinks of something else they want to test.
> I'm imagining an API like
>
>           isolation_test_is_waiting_for(int, int[]) returns bool
>
> so that isolationtester's query would reduce to something like
>
>         SELECT pg_catalog.isolation_test_is_waiting_for($1, '{...}')
>
> which would take even more cycles out of the parse/plan overhead for it
> (which is basically what's killing the CCA animals).  Internally, this
> function would call pg_blocking_pids and, if necessary,
> GetSafeSnapshotBlockingPids, and would check for matches in its array
> argument directly; it could probably do that significantly faster than the
> general-purpose array && code.  So we'd have to expend a bit of backend C
> code, but we'd have something that would be quite speedy and we could
> customize in future without fear of breaking behavior that users are
> depending on.

Good suggestion.

Thomas, would you like to produce a patch along these lines, or
should I?

--
Kevin Grittner

[1]  Dan R. K. Ports and Kevin Grittner. 2012.
     Serializable Snapshot Isolation in PostgreSQL.
     Proceedings of the VLDB Endowment, Vol. 5, No. 12.
     The 38th International Conference on Very Large Data Bases,
     August 27th - 31st 2012, Istanbul, Turkey.
     http://vldb.org/pvldb/vol5/p1850_danrkports_vldb2012.pdf

  15. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-09T00:33:13Z

    Kevin Grittner <kgrittn@gmail.com> writes:
> On Sat, Apr 8, 2017 at 12:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm imagining an API like
>> 	isolation_test_is_waiting_for(int, int[]) returns bool

> Good suggestion.

> Thomas, would you like to produce a patch along these lines, or
> should I?

I'll be happy to review/push if someone else does a first draft.

			regards, tom lane

  16. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-09T00:53:54Z

    ... BTW, one other minor coding suggestion for
GetSafeSnapshotBlockingPids(): it might be better to avoid doing so much
palloc work while holding the SerializableXactHashLock.  Even if it's
only held shared, I imagine that it's a contention bottleneck.  You
could avoid that by returning an array rather than a list; the array
could be preallocated of size MaxBackends before ever taking the lock.
That would be a little bit space-wasteful, but since it's only short-lived
storage it doesn't seem like much of a problem.

			regards, tom lane

  17. Re: recent deadlock regression test failures

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-09T11:49:38Z

    On Sun, Apr 9, 2017 at 12:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> On Sat, Apr 8, 2017 at 12:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm imagining an API like
>>>      isolation_test_is_waiting_for(int, int[]) returns bool
>
>> Good suggestion.
>
>> Thomas, would you like to produce a patch along these lines, or
>> should I?
>
> I'll be happy to review/push if someone else does a first draft.

Ok, I'll post a new patch tomorrow.

-- 
Thomas Munro
http://www.enterprisedb.com

  18. Re: recent deadlock regression test failures

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-10T04:15:18Z

    On Sun, Apr 9, 2017 at 11:49 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Sun, Apr 9, 2017 at 12:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Kevin Grittner <kgrittn@gmail.com> writes:
>>> On Sat, Apr 8, 2017 at 12:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I'm imagining an API like
>>>>      isolation_test_is_waiting_for(int, int[]) returns bool
>>
>>> Good suggestion.
>>
>>> Thomas, would you like to produce a patch along these lines, or
>>> should I?
>>
>> I'll be happy to review/push if someone else does a first draft.
>
> Ok, I'll post a new patch tomorrow.

Here's a pair of draft patches for review:

1.  pg-isolation-test-session-is-blocked.patch, to fix the
CACHE_CLOBBERED_ALWAYS problem by doing all the work in C as
suggested.

2.  pg-safe-snapshot-blocking-pids.patch, to provide an end-user
function wrapping GetSafeSnapshotBlockingPids().  Kevin expressed an
interest in that functionality, and it does seem useful: for example,
someone might use it to investigate which backends are holding up
pg_dump --serializable-deferrrable.  This is a separate patch because
you might consider it material for the next cycle, though at least
it's handy for verifying that GetSafeSnapshotBlockingPids() is working
correctly.  To test, open some number of SERIALIZABLE transactions and
take a snapshot in each, and then open a SERIALIZABLE READ ONLY
DEFERRABLE transaction and try to take a snapshot; it will block and
pg_safe_snapshot_blocking_pids() will identify the culprit(s).

Thanks to Andrew Gierth for an off-list discussion about the pitfalls
of trying to use "arrayoverlap" from inside
pg_isolation_test_session_is_blocked, which helped me decide that I
should open-code that logic instead.  Does that make sense?

-- 
Thomas Munro
http://www.enterprisedb.com

  19. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-10T04:19:38Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Here's a pair of draft patches for review:

I'll look at these in detail tomorrow, but:

> 2.  pg-safe-snapshot-blocking-pids.patch, to provide an end-user
> function wrapping GetSafeSnapshotBlockingPids().  Kevin expressed an
> interest in that functionality, and it does seem useful: for example,
> someone might use it to investigate which backends are holding up
> pg_dump --serializable-deferrrable.  This is a separate patch because
> you might consider it material for the next cycle, though at least
> it's handy for verifying that GetSafeSnapshotBlockingPids() is working
> correctly.

Personally I have no problem with adding this now, even though it
could be seen as a new feature.  Does anyone want to lobby against?

			regards, tom lane

  20. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-10T14:28:20Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Here's a pair of draft patches for review:

Pushed with cosmetic improvements.

I notice that the safe-snapshot code path is not paying attention to
parallel-query cases, unlike the lock code path.  I'm not sure how
big a deal that is...

			regards, tom lane

  21. Re: recent deadlock regression test failures

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-10T17:45:10Z

    On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> Here's a pair of draft patches for review:

Thanks.

> Pushed with cosmetic improvements.

Thanks.

> I notice that the safe-snapshot code path is not paying attention to
> parallel-query cases, unlike the lock code path.  I'm not sure how
> big a deal that is...

Parallel workers in serializable transactions should be using the
transaction number of the "master" process to take any predicate
locks, and if parallel workers are doing any DML work against
tuples, that should be using the master transaction number for
xmin/xmax and serialization failure testing.  If either of those
rules are being violated, parallel processing should probably be
disabled within a serializable transaction.  I don't think
safe-snapshot processing needs to do anything special if the above
rules are followed, nor can I see anything special it *could* do.

--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/

  22. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-10T18:17:22Z

    Kevin Grittner <kgrittn@gmail.com> writes:
> On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I notice that the safe-snapshot code path is not paying attention to
>> parallel-query cases, unlike the lock code path.  I'm not sure how
>> big a deal that is...

> Parallel workers in serializable transactions should be using the
> transaction number of the "master" process to take any predicate
> locks, and if parallel workers are doing any DML work against
> tuples, that should be using the master transaction number for
> xmin/xmax and serialization failure testing.

Right, but do they record the master's PID rather than their own in
the SERIALIZABLEXACT data structure?

Maybe it's impossible for a parallel worker to acquire its own
snapshot at all, in which case this is moot.  But I'm nervous.

			regards, tom lane

  23. Re: recent deadlock regression test failures

    Kevin Grittner <kgrittn@gmail.com> — 2017-04-10T18:45:12Z

    On Mon, Apr 10, 2017 at 1:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I notice that the safe-snapshot code path is not paying attention to
>>> parallel-query cases, unlike the lock code path.  I'm not sure how
>>> big a deal that is...
>
>> Parallel workers in serializable transactions should be using the
>> transaction number of the "master" process to take any predicate
>> locks, and if parallel workers are doing any DML work against
>> tuples, that should be using the master transaction number for
>> xmin/xmax and serialization failure testing.
>
> Right, but do they record the master's PID rather than their own in
> the SERIALIZABLEXACT data structure?

There had better be just a single SERIALIZABLEXACT data structure
for a serializable transaction, or it just doesn't work.  I can't
see how it would have anything but the master's PID in it.  If
parallel execution is possible in a serializable transaction and
things are done any other way, we have a problem.

> Maybe it's impossible for a parallel worker to acquire its own
> snapshot at all, in which case this is moot.  But I'm nervous.

I have trouble seeing what sane semantics we could possibly have if
each worker for a parallel scan used a different snapshot -- under
any transaction isolation level.  I know that under the read
committed transaction isolation level we can follow xmax pointers to
hit tuples on the target relation which are not in the snapshot used
for the scan, but I don't see how that would negate the need to have
the scan itself run on a single snapshot,

--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/

  24. Re: recent deadlock regression test failures

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-10T18:49:10Z

    On Tue, Apr 11, 2017 at 6:17 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I notice that the safe-snapshot code path is not paying attention to
>>> parallel-query cases, unlike the lock code path.  I'm not sure how
>>> big a deal that is...
>
>> Parallel workers in serializable transactions should be using the
>> transaction number of the "master" process to take any predicate
>> locks, and if parallel workers are doing any DML work against
>> tuples, that should be using the master transaction number for
>> xmin/xmax and serialization failure testing.
>
> Right, but do they record the master's PID rather than their own in
> the SERIALIZABLEXACT data structure?
>
> Maybe it's impossible for a parallel worker to acquire its own
> snapshot at all, in which case this is moot.  But I'm nervous.

Parallel workers can't acquire snapshots, and SERIALIZABLE disables
all parallel query planning anyway.

I did some early stage POC hacking to lift that restriction[1], and if
we finish up doing it at all like that then all SERIALIZABLEXACT
structures would be associated with leader processes and
pg_safe_snapshot_blocking_pid() would automatically deal only in
leader PIDs as arguments and results so there would be no problem
here.  The interlocking I proposed in that WIP patch may need work, to
be discussed, but I'm fairly sure that sharing the leader's
SERIALIZABLEXACT like that is the only sensible way forward.

[1] https://commitfest.postgresql.org/14/1004/

-- 
Thomas Munro
http://www.enterprisedb.com

  25. Re: recent deadlock regression test failures

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-10T19:10:50Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Tue, Apr 11, 2017 at 6:17 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Maybe it's impossible for a parallel worker to acquire its own
>> snapshot at all, in which case this is moot.  But I'm nervous.

> Parallel workers can't acquire snapshots, and SERIALIZABLE disables
> all parallel query planning anyway.

> I did some early stage POC hacking to lift that restriction[1], and if
> we finish up doing it at all like that then all SERIALIZABLEXACT
> structures would be associated with leader processes and
> pg_safe_snapshot_blocking_pid() would automatically deal only in
> leader PIDs as arguments and results so there would be no problem
> here.  The interlocking I proposed in that WIP patch may need work, to
> be discussed, but I'm fairly sure that sharing the leader's
> SERIALIZABLEXACT like that is the only sensible way forward.

OK, sounds good.  I agree that it would only be sensible for a parallel
worker to be using a snapshot already acquired by the master, so far
as the parallelized query itself is concerned.  What was worrying me
is snapshots acquired by, eg, volatile PL functions called by the query.
But on second thought, in SERIALIZABLE mode no such function would be
taking an actual new snapshot anyhow --- they'd just continue to use
the transaction snap.

			regards, tom lane

  26. Re: recent deadlock regression test failures

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-11T06:25:26Z

    On Tue, Apr 11, 2017 at 5:45 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Mon, Apr 10, 2017 at 9:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>>> Here's a pair of draft patches for review:
>
> Thanks.
>
>> Pushed with cosmetic improvements.
>
> Thanks.

Thanks Tom and Kevin.

After this commit hyrax ran the isolation test in 00:56:14, shaving
10-15 minutes off the typical earlier times, and is green once again.

-- 
Thomas Munro
http://www.enterprisedb.com