Re: BUG #14941: Vacuum crashes

On Tue, Dec 19, 2017 at 7:18 AM, Bossart, Nathan <bossartn@amazon.com> wrote:
> On 12/18/17, 3:30 PM, "Masahiko Sawada" <sawada.mshk@gmail.com> wrote:
>> According to the following old comment, there might be reason why we
>> didn't pass the information to vacuum_rel(). But your patch fetches
>> the relation
>> name even if the "relation" is not provided. I wonder if it can be
>> problem in some cases.
>
> Thanks for taking another look.
>
> I've thought through a few edge cases here, but I haven't noticed
> anything that I think is a problem.  If an unspecified relation is
> renamed prior to get_rel_name(), we'll use the updated name, which
> doesn't seem like an issue.  If an unspecified relation is renamed
> between get_rel_name() and the log statement, we'll use the old name,
> which seems possible in the current logging logic for VACUUM/ANALYZE.
> And if an unspecified relation is dropped just prior to
> get_rel_name(), the result will be NULL, and the logging will be
> skipped (as it already is for concurrently dropped relations that are
> not specified in the command).
>

Thank you for explanation.

There are three cases where "relation" is set NULL:
 * Vacuum to whole database
 * Child tables when vacuum to parent table
 * TOAST tables when vacuum to normal table
As current related comment says, it would not be appropriate to
complain if we could not open such unspecified relations later. But
with you patch, we would complain it only if we could not  take a lock
on these relations. It's fine with me.

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.
         */

Regards,

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