Re: BUG #14941: Vacuum crashes

On 2018-01-11 08:14:42 +0900, Michael Paquier wrote:
> On Wed, Jan 10, 2018 at 05:26:43PM +0000, Bossart, Nathan wrote:
> > Perhaps any logging changes for VACOPT_NOWAIT could be handled in a
> > separate thread. For now, I've updated 0003 to remove the logging
> > changes.
>
> Thanks. I am marking those as ready for committer, you are providing the
> set patch patch which offer the most consistent experience.

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.

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.

Lightly edited patches attached. Please preserve commit messages while
fixing these issues.

Greetings,

Andres Freund