Re: BUG #14941: Vacuum crashes

Thanks for taking a look.

On 3/3/18, 6:13 PM, "Andres Freund" <andres@anarazel.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