Re: BUG #14941: Vacuum crashes

On 12/21/17, 11:07 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:
> On the latest patch, it looks good to me except for a following comment.
>
> -                * If the RangeVar is not defined, we do not have
> enough information
> -                * to provide a meaningful log statement.  Chances are that
> -                * vacuum_rel's caller has intentionally not provided
> this information
> -                * so that this logging is skipped, anyway.
> +                * If relname is NULL, we do not have enough
> information to provide a
> +                * meaningful log statement.  Chances are that this
> information was
> +                * intentionally not provided so that this logging is
> skipped, anyway.
>
> This comment looks odd to me because we ourselves didn't set relname
> just before. For example, we can move the sentence to above comment
> block as follows. Thoughts?
>
>         /*
>          * If relation is NULL, we do not have enough information to provide a
>          * meaningful log statement. Chances are that this information was
>          * intentionally not provided so that this logging is skipped, anyway.
>          * However, iff VACOPT_NOWAIT is specified, we always try to emit
>          * a log statement even if relation is NULL.
>          */
>
> (snip)
>
>         /*
>          * Determine the log level.
>          *
>          * For autovacuum logs, we emit a LOG if
>          * log_autovacuum_min_duration is not diabled. For manual VACUUM, we
>          * emit a WARNING to match the log statements in the permission
>          * checks.
>          */

I agree, this makes more sense.  I've made this change in v3 of 0003.

Nathan