Re: BUG #14941: Vacuum crashes - Mailing list pgsql-hackers

On Mon, Dec 11, 2017 at 2:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Dec 6, 2017 at 1:52 AM, Bossart, Nathan <bossartn@amazon.com> wrote:
>> 0001_fix_unparenthesized_vacuum_grammar_v1.patch
>>
>> One VacuumStmt grammar rule allows users to specify the VACOPT_ANALYZE
>> option by including an AnalyzeStmt at the end of the command.  This
>> appears to have been added as part of the introduction of the ANALYZE
>> command (f905d65e).  However, this means that users can call VACUUM
>> commands like
>>
>>         VACUUM VERBOSE ANALYZE VERBOSE pg_class;
>>
>> Besides being disallowed according to the documentation, I think this
>> may give users the idea that there is a difference between the VERBOSE
>> options for VACUUM versus ANALYZE.  In fact, they are the same option,
>> and specifying it twice is redundant.
>
> Hmm. Yeah. If adding a parenthesis grammar to ANALYZE this is a good
> move for long purposes. If we don't do that now, or at least for the
> purpose of this patch set, then a AnalyzeStmt node embedded directly
> in the grammar of VACUUM is likely to bite in the future.
>
>> This change fixes the unparenthesized VACUUM syntax to disallow the out-
>> of-order VACUUM options as described above.  This is a prerequisite
>> change for opening up VACOPT_NOWAIT to users, as adding it to the
>> grammar as-is would cause statements like
>>
>>         VACUUM VERBOSE NOWAIT ANALYZE VERBOSE NOWAIT pg_class;
>>
>> to be allowed.  Since I am also looking to introduce a parenthesized
>> syntax for ANALYZE, this patch would prevent statements like
>
> If you add only a parenthesized grammar of ANALYZE, it seems to me
> that this would not be an allowed query anyway.
>
>>         VACUUM VERBOSE ANALYZE (VERBOSE, NOWAIT) pg_class;
>>
>> from being accepted as well.
>
> This could also be equivalent to ANALYZE(VERBOSE, NOWAIT). I can see
> pros and cons by keeping the complete extension of AnalyzeStmt in the
> VACUUM grammar, but as the documentation does not mention VACUUM
> VERBOSE ANALYZE VERBOSE as a legit query I guess that ripping it out
> is not going to annoy many users. Still let's see opinions from other
> folks on -hackers as another approach to the feature you want to add
> here would be to just implement the grammar for VACUUM but *not*
> ANALYZE, but I'd like to think that we still want ANALYZE to be
> supported, and also we want it to be extensible as a different thing
> than what VACUUM is.
>
>> 0002_add_parenthesized_analyze_syntax_v1.patch
>>
>> As a prerequisite to giving users access to the VACOPT_NOWAIT option, I
>> am introducing a parenthesized syntax for ANALYZE that is similar to
>> VACUUM's.  Following VACUUM's example, any new options will be added to
>> this syntax, and the old style will become deprecated.
>>
>> Adding a parenthesized syntax for ANALYZE is not strictly necessary for
>> providing the NOWAIT option, as NOWAIT is already a keyword in gram.y.
>> However, I thought it would be good to match VACUUM (since the command
>> structure is so similar), and this work would be required at some point
>> anyway if ANALYZE ever accepts options that we do not want to make
>> reserved keywords.
>
> Yep, agreed with the contents of this patch.
>
>> 0003_add_nowait_vacuum_option_v1.patch
>>
>> This change provides the existing VACOPT_NOWAIT option to users.  This
>> option was previously only used by autovacuum in special cases, but it
>> seems like a potentially valuable option in other cases as well.  For
>> example, perhaps a user wants to run a nightly VACUUM job that skips
>> tables that cannot be locked due to autovacuum (or something else)
>> already working on it.
>>
>> I chose to use NOWAIT as the option name because it is already a keyword
>> for the LOCK command.
>
> Makes sense to me.
>
>> This patch follows the existing logging precedent added by 11d8d72c and
>> ab6eaee8: if a relation is explicitly specified in the command, a log
>> statement will be emitted if it is skipped.  If the relation is not
>> specified (e.g. "VACUUM FULL;"), no such log statements will be emitted.
>
> +WARNING:  skipping analyze of "test1" --- lock not available
> +step analyze_specified: ANALYZE (NOWAIT) test1, test2;
> +step commit:
> +   COMMIT;
> OK for a WARNING for me in this case. We already do that for the other entries.

I also reviewed the patches.

For 0003 patch, if we do VACUUM (NOWAIT) to whole database we don't
get the above WARNING messages, but I think we should get them. The
reported issue also did VACUUM FULL and REINDEX to whole database.
Especially when VACUUM to whole database the information of skipped
relations would be useful for users.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center