Re: BUG #14941: Vacuum crashes

From: "Bossart, Nathan" <bossartn(at)amazon(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Lyes Ameddah <lyes(dot)amd(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, "pgsql-bugs(at)postgresql(dot)org" <pgsql-bugs(at)postgresql(dot)org>
Subject: Re: BUG #14941: Vacuum crashes
Date: 2018-03-05 21:55:13
Message-ID: 7327B413-1A57-477F-A6A0-6FD80CB5482D@amazon.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-bugs pgsql-hackers

Thanks for taking a look.

On 3/3/18, 6:13 PM, "Andres Freund" <andres(at)anarazel(dot)de> wrote:
> I was working on committing 0002 and 0003, when I noticed that the
> second patch doesn't actually fully works. NOWAIT does what it says on
> the tin iff the table is locked with a lower lock level than access
> exclusive. But if AEL is used, the command is blocked in
>
> static List *
> expand_vacuum_rel(VacuumRelation *vrel)
> ...
> /*
> * We transiently take AccessShareLock to protect the syscache lookup
> * below, as well as find_all_inheritors's expectation that the caller
> * holds some lock on the starting relation.
> */
> relid = RangeVarGetRelid(vrel->relation, AccessShareLock, false);
>
> ISTM has been added after the patches initially were proposed. See
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=19de0ab23ccba12567c18640f00b49f01471018d
>
> I'm a bit disappointed that the tests didn't catch this, they're clearly
> not fully there. They definitely should test the AEL case, as
> demonstrated here.

Sorry about that. I've added logic to handle ACCESS EXCLUSIVE in v6 of 0002,
and I've extended the tests to cover that case.

> Independent of that, I'm also concerned that NOWAIT isn't implemented
> consistently with other commands. Aren't we erroring out in other uses
> of NOWAIT? ISTM a more appropriate keyword would have been SKIP
> LOCKED. I think the behaviour makes sense, but I'd rename the internal
> flag and the grammar to use SKIP LOCKED.

Agreed. I've updated 0002 to use SKIP LOCKED instead of NOWAIT.

Nathan

Attachment Content-Type Size
v5-0001-Add-parenthesized-options-syntax-for-ANALYZE.patch application/octet-stream 4.7 KB
v6-0002-Add-NOWAIT-option-to-VACUUM-and-ANALYZE.patch application/octet-stream 17.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Magnus Hagander 2018-03-05 21:55:46 Re: 2018-03 CFM
Previous Message Andres Freund 2018-03-05 21:37:22 Re: JIT compiling with LLVM v11

Browse pgsql-bugs by date

  From Date Subject
Next Message PG Bug reporting form 2018-03-05 23:05:47 BUG #15099: View Filtered Rows
Previous Message Tom Lane 2018-03-05 19:38:33 Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column