Re: BUG #14941: Vacuum crashes

On 3/6/18, 11:04 PM, "Michael Paquier" <michael(at)paquier(dot)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