Re: BUG #14941: Vacuum crashes

On Wed, Dec 6, 2017 at 1:52 AM, Bossart, Nathan <bossartn(at)amazon(dot)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.

Let's see what other folks think first about the ANALYZE grammar in
VACUUM, as well as having ANALYZE use a parenthesized grammer. FWIW, I
like the contents of this patch set, and the thing is non-intrusive. I
think that NOWAIT gains more value now that multiple relations can be
vacuumed or analyzed with a single manual command.
--
Michael