Re: BUG #14941: Vacuum crashes

Nathan Bossart <bossartn@amazon.com>

From: "Bossart, Nathan" <bossartn@amazon.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>, Michael Paquier <michael.paquier@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Robert Haas" <robertmhaas@gmail.com>, Lyes Ameddah <lyes.amd@gmail.com>, "Tomas Vondra" <tomas.vondra@2ndquadrant.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, "pgsql-bugs@postgresql.org" <pgsql-bugs@postgresql.org>
Date: 2018-03-07T15:32:33Z
Lists: pgsql-bugs, pgsql-hackers
On 3/6/18, 11:04 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> +       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

I had missed that thread.  Thanks for pointing it out.

> +       /*
> +        * 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?

At the moment, stmttype is either "VACUUM" or "ANALYZE", but I suppose
there still might be multi-byte risk in translations.

> +       /*
> +        * 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().

Will do.

> +      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?

IMO that is already covered by saying the relation is skipped,
although I'm not against explicitly stating it, too.  Perhaps we could
outline the logging behavior as well.

Nathan

Commits

  1. Add option SKIP_LOCKED to VACUUM and ANALYZE

  2. Rename VACOPT_NOWAIT to VACOPT_SKIP_LOCKED

  3. Add parenthesized options syntax for ANALYZE.

  4. Don't allow VACUUM VERBOSE ANALYZE VERBOSE.

  5. Fix inadequate locking during get_rel_oids().

  6. Avoid having autovacuum workers wait for relation locks.