Re: BUG #14941: Vacuum crashes

On 1/8/18, 10:28 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote:
> Based on the opinions gathered on this thread, 0001 and 0002 seem to be
> in the shape wanted, and those look good for shipping. Now for 0003 we
> are not there yet.

Thanks for taking a look.

> -       if (relation == NULL)
> +       if (relation != NULL)
> +           relname = relation->relname;
> +       else if (!rel_lock)
> +           relname = get_rel_name(relid);
> +
> +       if (relname == NULL)
> I think that you are doing it wrong here. In get_all_vacuum_rels() you
> should build a RangeVar to be reused in the context of this error
> message, and hence you'll save an extra lookup based on the relation
> OID here, saving from any timing issues that you have overseen as in
> this code path a lock on the relation whose name is looked at is not
> taken. Relying on the RangeVar being NULL to not generate any logs is
> fine as a concept to me, but let's fill it where it is needed, and in
> the case of this patch a VACUUM NOWAIT on the whole database is such a
> case.

I understand what you are saying here.  I think there are two competing
logging behaviors:

  1. If a relation is concurrently dropped, we should skip logging if
     the relation was not specified in the original VACUUM/ANALYZE
     command [0]
  2. If a relation is skipped because the lock is not available, we
     should never skip logging

Since we rely on the RangeVar being NULL to determine whether or not
we should log for case 1, filling in the RangeVar during
get_all_vacuum_rels() would add complexity.  Any time VACOPT_NOWAIT is
specified, we must construct a RangeVar for all relations.  Also, we
must find a new way to track whether or not the relation was specified
in the original command in order to maintain the behavior of case 1.

IMO it is simpler to call get_rel_name() to discover the relation name
in this specific case (NOWAIT is specified, lock is not available, and
the relation was not specified in the original command).  I detailed
some potential edge cases of this approach earlier [1].  Even if I am
missing some, I think the worst case is that get_rel_name() returns
NULL and the logging is skipped.

What do you think?  I will take a deeper look into how your suggested
approach might be achieved.

Nathan

[0] https://postgr.es/m/28748.1507071576%40sss.pgh.pa.us
[1] https://postgr.es/m/32F4B778-292B-41AF-891A-497B8127CD59%40amazon.com