From: Tom Lane <tgl@sss.pgh.pa.us>
To: Masahiko Sawada <sawada.mshk@gmail.com>
cc: Robert Haas <robertmhaas@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: 
 <CAD21AoAjw_wTZA5vxjBM7BaOTaAHdUdCSui-MtcDwYsY1p25HA@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>
 <15751.1555256860@sss.pgh.pa.us>
 <CAD21AoCZpFLC-KZywkC-K7OVp0PJKey6znaukTdTv4sEdJWQNQ@mail.gmail.com>
 <CAD21AoAjw_wTZA5vxjBM7BaOTaAHdUdCSui-MtcDwYsY1p25HA@mail.gmail.com>
Comments: In-reply-to Masahiko Sawada <sawada.mshk@gmail.com>
	message dated "Mon, 15 Apr 2019 15:07:24 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <16813.1555348381.1@sss.pgh.pa.us>
Date: Mon, 15 Apr 2019 13:13:01 -0400
Message-ID: <16814.1555348381@sss.pgh.pa.us>
Precedence: bulk

Masahiko Sawada <sawada.mshk@gmail.com> writes:
>> 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.

> Here is a draft patch to fix this issue.

So the real issue here, I fear, is that we've got no consistent testing
of the whole case block for HEAPTUPLE_DEAD, as you can easily confirm
by checking the code coverage report at
https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html

This is perhaps unsurprising, given the code comment that points out that
we can only reach that block if the tuple's state changed since
heap_page_prune() a few lines above.  Still, it means that this patch
hasn't been tested in that scenario, until we were lucky enough for
a slow buildfarm machine like topminnow to hit it.

What's more, because that block is the only way for "tupgone" to be
set, we also don't reach the "if (tupgone)" block at lines 1183ff.
And I think this patch has probably broken that, too.  Surely, if we
are not going to remove the tuple, we should not increment tups_vacuumed?
And maybe we have to freeze it instead?  How is it that we can, or should,
treat this situation as different from a dead-but-not-removable tuple?

I have a very strong feeling that this patch was not fully baked.

BTW, I can reproduce the crash fairly easily (within a few cycles
of the regression tests) by (a) inserting pg_usleep(10000) after
the heap_page_prune call, and (b) making autovacuum much more
aggressive.

			regards, tom lane