pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: Bossart, Nathan <bossartn@amazon.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: New vacuum option to do only freezing
Date: Mon, 15 Apr 2019 13:13:01 -0400
Message-ID: <16814.1555348381@sss.pgh.pa.us> (raw)
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>

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





view thread (118+ messages)  latest in thread

Message-ID: <16814.1555348381@sss.pgh.pa.us>
Permalink:  ../16814.1555348381@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/16814.1555348381@sss.pgh.pa.us

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, sawada.mshk@gmail.com, robertmhaas@gmail.com, horiguchi.kyotaro@lab.ntt.co.jp, bossartn@amazon.com, alvherre@2ndquadrant.com
  Subject: Re: New vacuum option to do only freezing
  In-Reply-To: <16814.1555348381@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox