Thread: Autovacuum cancellation
Simon Riggs wrote: > Just noticed you've made these changes. I was working on them, but > hadn't fully tested the patch because of all the different touch points. > Sorry for the delay. > > Would you like me to refresh my earlier patch against the newly > committed state (or did you commit that aspect already as well?). Patch attached, please comment. It only avoids cancelling when the vacuum is for wraparound. What we're missing here is doc updates (mainly to lmgr/README, I think) -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Attachment
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Patch attached, please comment. It only avoids cancelling when the
> vacuum is for wraparound.
I'm not entirely convinced that there can be only one autovac proc in
the portion of the waits-for graph explored by DeadlockCheck. If there
is more than one, we'll cancel a random one of them, which seems OK ---
but the comment added to FindLockCycleRecurse is bogus.
I thought about suggesting that we test PROC_VACUUM_FOR_WRAPAROUND
before setting blocking_autovacuum_proc at all, but I guess the reason
you don't do that is you don't want to take ProcArrayLock there (and we
decided it was unsafe to check the bit without the lock). So the other
thing that comment block needs is a note that it seems OK to check
PROC_IS_AUTOVACUUM without the lock, because it never changes for an
existing PGPROC, but not PROC_VACUUM_FOR_WRAPAROUND.
Otherwise it looks OK --- a bit ugly but I don't have a better idea.
There's some things still to be desired here: if an autovac process is
involved in a hard deadlock, the patch doesn't favor zapping it over
anybody else, nor consider cancelling the autovac as an alternative to
rearranging queues for a soft deadlock. But dealing with that will open
cans of worms that I don't think we want to open for 8.3.
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > There's some things still to be desired here: if an autovac process is > involved in a hard deadlock, the patch doesn't favor zapping it over > anybody else, nor consider cancelling the autovac as an alternative to > rearranging queues for a soft deadlock. But dealing with that will open > cans of worms that I don't think we want to open for 8.3. Can autovacuum actually get into a hard deadlock? Does it take more than one lock that can block others at the same time? I think there's a window where the process waiting directly on autovacuum could have already fired its deadlock check before it was waiting directly on autovacuum. But the only way I can see it happening is if another process is cancelled before its deadlock check fires and the signals are processed out of order. I'm not sure that's a case we really need to worry about. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
> Can autovacuum actually get into a hard deadlock?
It can certainly be part of a deadlock loop, though the practical cases
might be few. It will be holding more than one lock, eg a lock on its
target table and various transient locks on system catalogs, and other
processes taking or trying for exclusive locks on those things could
create issues. I think it's OK to leave the issue go for now, until
we see if anyone hits it in practice --- I just wanted to mention that
we might need to consider the problem in future.
> I think there's a window where the process waiting directly on
> autovacuum could have already fired its deadlock check before it was
> waiting directly on autovacuum.
I think you don't understand what that code is doing. If there's an
autovac anywhere in the dependency graph, it'll find it.
regards, tom lane
Tom Lane wrote: > Gregory Stark <stark@enterprisedb.com> writes: > > I think there's a window where the process waiting directly on > > autovacuum could have already fired its deadlock check before it was > > waiting directly on autovacuum. > > I think you don't understand what that code is doing. If there's an > autovac anywhere in the dependency graph, it'll find it. Thanks for making that explicit, I was trying to build a test case where it would lock :-) (If we had concurrent psql we could even have it in the regression tests ...) -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > /* > * Look for a blocking autovacuum. There will only ever > * be one, since the autovacuum workers are careful > * not to operate concurrently on the same table. > */ I think that's a bit unaccurate. You could have multiple autovacuum workers operating on different tables participating in a deadlock. The reason that can't happen is that autovacuum never holds a lock while waiting for another. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
"Tom Lane" <tgl@sss.pgh.pa.us> writes: >> I think there's a window where the process waiting directly on >> autovacuum could have already fired its deadlock check before it was >> waiting directly on autovacuum. > > I think you don't understand what that code is doing. If there's an > autovac anywhere in the dependency graph, it'll find it. That'll teach me to try to read code from a patch directly without trying to apply it or at least read the original source next to it. I thought I had seen this code recently enough to apply the patch from memory -- clearly not. I assume the right thing happens if multiple deadlock check signals fire for the same autovacuum? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki@enterprisedb.com> writes:
> Alvaro Herrera wrote:
>> /*
>> * Look for a blocking autovacuum. There will only ever
>> * be one, since the autovacuum workers are careful
>> * not to operate concurrently on the same table.
>> */
> I think that's a bit unaccurate. You could have multiple autovacuum
> workers operating on different tables participating in a deadlock. The
> reason that can't happen is that autovacuum never holds a lock while
> waiting for another.
And that's not true either. It may only want low-grade locks (eg
AccessShareLock on a system catalog) but deadlock is nonetheless
entirely possible in principle.
regards, tom lane
Gregory Stark <stark@enterprisedb.com> writes:
> I assume the right thing happens if multiple deadlock check signals fire for
> the same autovacuum?
Multiple signals shouldn't be a problem, but late-arriving ones could be.
It might be worth having autovac explicitly clear QueryCancelPending
after it's finished a table, so that a SIGINT sent because of activity
on one table couldn't force cancellation of vacuum on the next one.
regards, tom lane
Tom Lane wrote: > Gregory Stark <stark@enterprisedb.com> writes: > > I assume the right thing happens if multiple deadlock check signals fire for > > the same autovacuum? > > Multiple signals shouldn't be a problem, but late-arriving ones could be. > It might be worth having autovac explicitly clear QueryCancelPending > after it's finished a table, so that a SIGINT sent because of activity > on one table couldn't force cancellation of vacuum on the next one. Ok, committed; I snuck that in as well, but I'm not sure how to test that it works. I adjusted the comments -- I think they're more correct now. I also added a puny paragraph to lmgr/README. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, 2007-10-25 at 17:35 -0400, Tom Lane wrote: > There's some things still to be desired here: if an autovac process is > involved in a hard deadlock, the patch doesn't favor zapping it over > anybody else, nor consider cancelling the autovac as an alternative to > rearranging queues for a soft deadlock. But dealing with that will > open cans of worms that I don't think we want to open for 8.3. I did look at doing that but decided it would not be appropriate to do that in all cases. i.e. there are hard deadlock cases where the autovac can be the head of the lock queue and yet a deadlock still exists between two other processes. The deadlock detector doesn't get called twice for the same deadlock, so it wasn't possible to speculatively do that and then re-catch it second time around. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
On Fri, 2007-10-26 at 10:32 +0100, Heikki Linnakangas wrote: > Alvaro Herrera wrote: > > /* > > * Look for a blocking autovacuum. There will only ever > > * be one, since the autovacuum workers are careful > > * not to operate concurrently on the same table. > > */ > > I think that's a bit unaccurate. You could have multiple autovacuum > workers operating on different tables participating in a deadlock. The > reason that can't happen is that autovacuum never holds a lock while > waiting for another. I wrote that code comment; as you say it is true only when there are at least 4 processes in the lock graph where 2+ normal backends are deadlocking and there are 2+ autovacuums holding existing locks. The comment should have said "If blocking is caused by an autovacuum process then ... (there will)". The blocking_autovacuum_proc doesn't react unless there are no hard deadlocks, so the code works. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
On Fri, 2007-10-26 at 17:50 -0300, Alvaro Herrera wrote: > Ok, committed; I snuck that in as well, but I'm not sure how to test > that it works. I've had time to review that now. I didn't reply to your original post because you'd taken my name off the copy list for some reason and I've been too busy to read non-addressed mail. The committed patch is pretty much the same as my original AFAICS. I'm sure you didn't mean to forget that, but can you please acknowledge my contribution in CVS? Thanks. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
On Sat, 2007-10-27 at 23:22 +0100, Simon Riggs wrote: > On Fri, 2007-10-26 at 10:32 +0100, Heikki Linnakangas wrote: > > Alvaro Herrera wrote: > > > /* > > > * Look for a blocking autovacuum. There will only ever > > > * be one, since the autovacuum workers are careful > > > * not to operate concurrently on the same table. > > > */ > > > > I think that's a bit unaccurate. You could have multiple autovacuum > > workers operating on different tables participating in a deadlock. The > > reason that can't happen is that autovacuum never holds a lock while > > waiting for another. > > I wrote that code comment; as you say it is true only when there are at > least 4 processes in the lock graph where 2+ normal backends are > deadlocking and there are 2+ autovacuums holding existing locks. The > comment should have said "If blocking is caused by an autovacuum process > then ... (there will)". Sorry...this should read "as you say it is **not** true". -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com