Re: BUG #14941: Vacuum crashes

Michael Paquier <michael.paquier@gmail.com>

From: Michael Paquier <michael.paquier@gmail.com>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: 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-01-10T02:53:31Z
Lists: pgsql-bugs, pgsql-hackers
On Tue, Jan 09, 2018 at 09:40:50PM +0000, Bossart, Nathan wrote:
> On 1/8/18, 10:28 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote:
>> I think that you are doing it wrong here. In get_all_vacuum_rels() you
>> should build a RangeVar to be reused in the context of this error
>> message, and hence you'll save an extra lookup based on the relation
>> OID here, saving from any timing issues that you have overseen as in
>> this code path a lock on the relation whose name is looked at is not
>> taken. Relying on the RangeVar being NULL to not generate any logs is
>> fine as a concept to me, but let's fill it where it is needed, and in
>> the case of this patch a VACUUM NOWAIT on the whole database is such a
>> case.
> 
> I understand what you are saying here.  I think there are two competing
> logging behaviors:
> 
>   1. If a relation is concurrently dropped, we should skip logging if
>      the relation was not specified in the original VACUUM/ANALYZE
>      command [0]
>   2. If a relation is skipped because the lock is not available, we
>      should never skip logging

At the end this comes back to if the relation is explicitely listed or
not in the command specified by the user..

> What do you think?  I will take a deeper look into how your suggested
> approach might be achieved.

Backpedalling a bit on this point and coming back to this message from
Tom (https://www.postgresql.org/message-id/28748.1507071576%40sss.pgh.pa.us)
which you just cited. Why do we actually need to issue any WARNING
messages for unlisted relations? Contrary to what Sawada-san complained
upthread, it looks sane to me to not log anything if a relation is not
explicitely listed. So you should not get any warnings for a
database-wide VACUUM if a relation is dropped while the thing is
running, and what you proposed initially in
https://www.postgresql.org/message-id/D3FC73E2-9B1A-4DB4-8180-55F57D116B4E@amazon.com
is more simple, does not need to worry about any kind of timing issues,
and is consistent with autovacuum.
--
Michael

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.