Re: New vacuum option to do only freezing

On Mon, Apr 15, 2019 at 12:47 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Apr 3, 2019 at 10:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >> Attached the updated version patch.
>
> > Committed with a little bit of documentation tweaking.
>
> topminnow just failed an assertion from this patch:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=topminnow&dt=2019-04-14%2011%3A01%3A48
>
> The symptoms are:
>
> TRAP: FailedAssertion("!((params->index_cleanup == VACOPT_TERNARY_ENABLED && nleft_dead_tuples == 0 && nleft_dead_itemids == 0) || params->index_cleanup == VACOPT_TERNARY_DISABLED)", File: "/home/nm/farm/mipsel_deb8_gcc_32/HEAD/pgsql.build/../pgsql/src/backend/access/heap/vacuumlazy.c", Line: 1404)
> ...
> 2019-04-14 14:49:16.328 CEST [15282:5] LOG:  server process (PID 18985) was terminated by signal 6: Aborted
> 2019-04-14 14:49:16.328 CEST [15282:6] DETAIL:  Failed process was running: autovacuum: VACUUM ANALYZE pg_catalog.pg_depend
>
> Just looking at the logic around index_cleanup, I rather think that
> that assertion is flat out wrong:
>
> +    /* No dead tuples should be left if index cleanup is enabled */
> +    Assert((params->index_cleanup == VACOPT_TERNARY_ENABLED &&
> +            nleft_dead_tuples == 0 && nleft_dead_itemids == 0) ||
> +           params->index_cleanup == VACOPT_TERNARY_DISABLED);
>
> Either it's wrong, or this is:
>
> +                        /*
> +                         * Since this dead tuple will not be vacuumed and
> +                         * ignored when index cleanup is disabled we count
> +                         * count it for reporting.
> +                         */
> +                        if (params->index_cleanup == VACOPT_TERNARY_ENABLED)
> +                            nleft_dead_tuples++;
>

Ugh, I think the assertion is right but the above condition is
completely wrong. We should increment nleft_dead_tuples when index
cleanup is *not* enabled. For nleft_dead_itemids we require that index
cleanup is disabled as follows.

           {
               /*
                * Here, we have indexes but index cleanup is disabled.
Instead of
                * vacuuming the dead tuples on the heap, we just forget them.
                *
                * Note that vacrelstats->dead_tuples could have tuples which
                * became dead after HOT-pruning but are not marked dead yet.
                * We do not process them because it's a very rare condition, and
                * the next vacuum will process them anyway.
                */
               Assert(params->index_cleanup == VACOPT_TERNARY_DISABLED);
               nleft_dead_itemids += vacrelstats->num_dead_tuples;
           }

Regards,

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