From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
cc: Masahiko Sawada <sawada.mshk@gmail.com>,
        Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
        "Bossart,
    Nathan" <bossartn@amazon.com>,
        Alvaro Herrera <alvherre@2ndquadrant.com>,
        PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: New vacuum option to do only freezing
In-reply-to: 
 <CA+TgmoZOJ=mPf4kedwg7HCHsG4Oimm67_3S9xEdYf=8nzO=a7w@mail.gmail.com>
References: <20190403.131702.71087088.horiguchi.kyotaro@lab.ntt.co.jp>
 <CAD21AoBtM=HGLkMKBgch37mf0-epa3_o=Y1PU0_m9r5YmtS-NQ@mail.gmail.com>
 <CA+Tgmoas581jpJ0TPaA38OhjXHgbLy8z1fuuHH7CaNkrboZJeA@mail.gmail.com>
 <20190404.091743.76975665.horiguchi.kyotaro@lab.ntt.co.jp>
 <CAD21AoDu0wHfBmgJSQdH4Yn1vJingqgdEQ4bUioJf8v=RQ9K=g@mail.gmail.com>
 <CA+TgmoZOJ=mPf4kedwg7HCHsG4Oimm67_3S9xEdYf=8nzO=a7w@mail.gmail.com>
Comments: In-reply-to Robert Haas <robertmhaas@gmail.com>
	message dated "Thu, 04 Apr 2019 15:05:55 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <15750.1555256860.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 14 Apr 2019 11:47:40 -0400
Message-ID: <15751.1555256860@sss.pgh.pa.us>
Precedence: bulk

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=3Dtopminnow&dt=3D2=
019-04-14%2011%3A01%3A48

The symptoms are:

TRAP: FailedAssertion("!((params->index_cleanup =3D=3D VACOPT_TERNARY_ENAB=
LED && nleft_dead_tuples =3D=3D 0 && nleft_dead_itemids =3D=3D 0) || param=
s->index_cleanup =3D=3D VACOPT_TERNARY_DISABLED)", File: "/home/nm/farm/mi=
psel_deb8_gcc_32/HEAD/pgsql.build/../pgsql/src/backend/access/heap/vacuuml=
azy.c", Line: 1404)
...
2019-04-14 14:49:16.328 CEST [15282:5] LOG:  server process (PID 18985) wa=
s 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 =3D=3D VACOPT_TERNARY_ENABLED &&
+            nleft_dead_tuples =3D=3D 0 && nleft_dead_itemids =3D=3D 0) ||
+           params->index_cleanup =3D=3D 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 coun=
t
+                         * count it for reporting.
+                         */
+                        if (params->index_cleanup =3D=3D VACOPT_TERNARY_E=
NABLED)
+                            nleft_dead_tuples++;

The poor quality of that comment suggests that maybe the code is just
as confused.

(I also think that that "ternary option" stuff is unreadably overdesigned
notation, which possibly contributed to getting this wrong.  If even the
author can't keep it straight, it's unreadable.)

			regards, tom lane