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+TgmoYJpnbk3XP2CHkzco95a-0XfpqUeCYm74mUMh2NZX5AxA@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>
 <16814.1555348381@sss.pgh.pa.us>
 <CA+TgmoYJpnbk3XP2CHkzco95a-0XfpqUeCYm74mUMh2NZX5AxA@mail.gmail.com>
Comments: In-reply-to Robert Haas <robertmhaas@gmail.com>
	message dated "Mon, 15 Apr 2019 15:34:44 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <23884.1555357618.1@sss.pgh.pa.us>
Date: Mon, 15 Apr 2019 15:46:58 -0400
Message-ID: <23885.1555357618@sss.pgh.pa.us>
Precedence: bulk

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 15, 2019 at 1:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I have a very strong feeling that this patch was not fully baked.

> I think you're right, but I don't understand the comment in the
> preceding paragraph.  How does this problem prevent tupgone from
> getting set?

My point is that I suspect that tupgone *shouldn't* get set.
It's not (going to be) gone.

> It looks to me like nleft_dead_tuples should be ripped out.

That was pretty much what I was thinking too.  It makes more sense
just to treat this case identically to dead-but-not-yet-removable.
I have substantial doubts about nleft_dead_itemids being worth
anything, as well.

> We should do something like:
> if (params->index_cleanup == VACOPT_TERNARY_DISABLED)
> {
>     nkeep += tups_vacuumed;
>     tups_vacuumed = 0;
> }

No.  I'm thinking there should be exactly one test of index_cleanup
in this logic, and what it would be is along the lines of

                    if (HeapTupleIsHotUpdated(&tuple) ||
                        HeapTupleIsHeapOnly(&tuple) ||
+                       params->index_cleanup == VACOPT_TERNARY_DISABLED)
                        nkeep += 1;
                    else

In general, this thing has a strong whiff of "large patch
with a small patch struggling to get out".

			regards, tom lane