On Mon, Apr 15, 2019 at 1:13 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Masahiko Sawada <sawada.m...@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.
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?
It looks to me like nleft_dead_tuples should be ripped out. It's
basically trying to count the same thing as tups_vacuumed, and there
doesn't seem to be any need to count that thing twice. And then just
below this block:
/* save stats for use later */
vacrelstats->tuples_deleted = tups_vacuumed;
vacrelstats->new_dead_tuples = nkeep;
vacrelstats->nleft_dead_itemids = nleft_dead_itemids;
We should do something like:
if (params->index_cleanup == VACOPT_TERNARY_DISABLED)
{
nkeep += tups_vacuumed;
tups_vacuumed = 0;
}
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
