Re: BUG #14941: Vacuum crashes

On Mon, Mar 05, 2018 at 09:55:13PM +0000, Bossart, Nathan wrote:
> 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.

Hm, yes.  I have also let those patches rot a bit myself.  Sorry for
that.

>> 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.

So, NOWAIT means "please try to take a lock, don't wait for it and issue
an error immediately if the lock cannot be taken".  SKIP_LOCKED means
"please try to take a lock, don't wait for it, but do not issue an error
if the lock cannot be taken and move on to next steps".  I agree that
this is more consistent.

+       if (!(options & VACOPT_SKIP_LOCKED))
+           relid = RangeVarGetRelid(vrel->relation, AccessShareLock,
false);
+       else
+       {
+           relid = RangeVarGetRelid(vrel->relation, NoLock, false);
Yeah, I agree with Andres that getting all this logic done in
RangeVarGetRelidExtended would be cleaner.  Let's see where the other
thread leads us to:
https://www.postgresql.org/message-id/20180306005349.b65whmvj7z6hbe2y%40alap3.anarazel.de

+       /*
+        * We must downcase the statement type for log message
consistency
+        * between expand_vacuum_rel(), vacuum_rel(), and analyze_rel().
+        */
+       stmttype_lower = asc_tolower(stmttype, strlen(stmttype));
This blows up on multi-byte characters and may report incorrect relation
name if double quotes are used, no?

+       /*
+        * Since autovacuum workers supply OIDs when calling vacuum(), no
+        * autovacuum worker should reach this code, and we can make
+        * assumptions about the logging levels we should use in the
checks
+        * below.
+        */
+       Assert(!IsAutoVacuumWorkerProcess());
This is a good idea, should be a separate patch as other folks tend to
be confused with relid handling in expand_vacuum_rel().

+      Specifies that <command>VACUUM</command> should not wait for any
+      conflicting locks to be released: if a relation cannot be locked
+      immediately without waiting, the relation is skipped
Should this mention as well that no errors are raised, allowing the
process to move on with the next relations?
--
Michael