Re: BUG #14941: Vacuum crashes

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