Обсуждение: [HACKERS] recent deadlock regression test failures
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
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
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
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
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
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
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
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
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
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
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
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
VLDBEndowment, 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
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
... 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
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
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
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
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/
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
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/
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
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
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