On Mon, Mar 05, 2018 at 09:55:13PM +0000, Bossart, Nathan wrote: > On 3/3/18, 6:13 PM, "Andres Freund" <[email protected]> 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
signature.asc
Description: PGP signature
