MIME-Version: 1.0
References: <CAH2-WzmT9N_42b=yuT-B03E=R48ECA6iEvR48305CSfwg=WUjw@mail.gmail.com>
 <CA+TgmoYhXCi6qWa6sU+6KFSD_Ha=PVJC7nPeXvhMF1poo2D=xg@mail.gmail.com>
 <CAH2-Wz=9Jr+s-hUbq=RxW8xRR8Ygb6OjpUDOVOK4z0qZe9Xopw@mail.gmail.com>
 <CA+Tgmobr_95jt2GwLoDYbE=t6ChXBvv3U48F8HJyjYUikY2Gnw@mail.gmail.com>
 <CAH2-Wzn11=HUK6gmTd1EjtZmEuXOYyifojpzPaVdVrxWFufTYA@mail.gmail.com>
 <CAH2-Wz=iLnf+0CsaB37efXCGMRJO1DyJw5HMzm7tp1AxG1NR2g@mail.gmail.com>
 <CAH2-WzkUBbg9SSL7bFWT_Y86027GAUJEgqwgbe2YjOLEpw9ysQ@mail.gmail.com>
 <CAH2-Wzk=uUHMPx3Zs41ouPBgxX-5BbwSuPeOBLyKt3y5jt-sgw@mail.gmail.com>
 <CA+TgmoZMKKO_dGBhEzZ5T8jbmL+3kxSuLoQx_pyDfHKutiifCg@mail.gmail.com>
 <CAH2-WzmfyfmmANV6RRchHmUBgjU9QYBUP_6CoFuQ3d23T4E6nQ@mail.gmail.com>
 <20220218221117.ijozsjwaxa6fy5u6@alap3.anarazel.de> <CAH2-Wz=DHb2hXGqcvuVyy0qC88VcYoaNjy2FFtb-xjQMOZTQqw@mail.gmail.com>
 <CAH2-WzkiB-qcsBmWrpzP0nxvrQExoUts1d7TYShg_DrkOHeg4Q@mail.gmail.com>
In-Reply-To: <CAH2-WzkiB-qcsBmWrpzP0nxvrQExoUts1d7TYShg_DrkOHeg4Q@mail.gmail.com>
From: Peter Geoghegan <pg@bowt.ie>
Date: Sat, 19 Feb 2022 16:22:23 -0800
Message-ID: <CAH2-WzkPb42odWen-HZRYw1zUe=fPAp1_JbSuVvPXQTO2iBMPw@mail.gmail.com>
Subject: Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="000000000000b883ee05d8681db8"
Archived-At: <https://www.postgresql.org/message-id/CAH2-WzkPb42odWen-HZRYw1zUe%3DfPAp1_JbSuVvPXQTO2iBMPw%40mail.gmail.com>
Precedence: bulk

--000000000000b883ee05d8681db8
Content-Type: text/plain; charset="UTF-8"

On Sat, Feb 19, 2022 at 3:08 PM Peter Geoghegan <pg@bowt.ie> wrote:
> It's quite possible that this is nothing more than a bug in my
> adversarial gizmo patch -- since I don't think that
> ConditionalLockBufferForCleanup() can ever fail with a temp buffer
> (though even that's not completely clear right now). Even if the
> behavior that I saw does not indicate a bug on HEAD, it still seems
> informative.

This very much looks like a bug in pg_surgery itself now -- attached
is a draft fix.

The temp table thing was a red herring. I found I could get exactly
the same kind of failure when htab2 was a permanent table (which was
how it originally appeared, before commit 0811f766fd made it into a
temp table due to test flappiness issues). The relevant "vacuum freeze
htab2" happens at a point after the test has already deliberately
corrupted one of its tuples using heap_force_kill().  It's not that we
aren't careful enough about the corruption at some point in
vacuumlazy.c, which was my second theory. But I quickly discarded that
idea, and came up with a third theory: the relevant heap_surgery.c
path does the relevant ItemIdSetDead() to kill items, without also
defragmenting the page to remove the tuples with storage, which is
wrong.

This meant that we depended on pruning happening (in this case during
VACUUM) and defragmenting the page in passing. But there is no reason
to not defragment the page within pg_surgery (at least no obvious
reason), since we have a cleanup lock anyway.

Theoretically you could blame this on lazy_scan_noprune instead, since
it thinks it can collect LP_DEAD items while assuming that they have
no storage, but that doesn't make much sense to me. There has never
been any way of setting a heap item to LP_DEAD without also
defragmenting the page.  Since that's exactly what it means to prune a
heap page. (Actually, the same used to be true about heap vacuuming,
which worked more like heap pruning before Postgres 14, but that
doesn't seem important.)

-- 
Peter Geoghegan

--000000000000b883ee05d8681db8
Content-Type: text/plain; charset="US-ASCII"; 
	name="0002-Fix-for-pg_surgery-s-heap_force_kill-function.txt"
Content-Disposition: attachment; 
	filename="0002-Fix-for-pg_surgery-s-heap_force_kill-function.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_kzuiqtvi0>
X-Attachment-Id: f_kzuiqtvi0

RnJvbSA4MWYwMWNhNjIzYjExNTY0N2VlNzhhMWIwOWJiYjQ0NThmYjM1ZGFiIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBHZW9naGVnYW4gPHBnQGJvd3QuaWU+CkRhdGU6IFNh
dCwgMTkgRmViIDIwMjIgMTY6MTM6NDggLTA4MDAKU3ViamVjdDogW1BBVENIIDIvMl0gRml4IGZv
ciBwZ19zdXJnZXJ5J3MgaGVhcF9mb3JjZV9raWxsKCkgZnVuY3Rpb24uCgotLS0KIGNvbnRyaWIv
cGdfc3VyZ2VyeS9oZWFwX3N1cmdlcnkuYyB8IDMgKystCiAxIGZpbGUgY2hhbmdlZCwgMiBpbnNl
cnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCgpkaWZmIC0tZ2l0IGEvY29udHJpYi9wZ19zdXJnZXJ5
L2hlYXBfc3VyZ2VyeS5jIGIvY29udHJpYi9wZ19zdXJnZXJ5L2hlYXBfc3VyZ2VyeS5jCmluZGV4
IDNlNjQxYWE2NC4uYTNhMTkzYmE1IDEwMDY0NAotLS0gYS9jb250cmliL3BnX3N1cmdlcnkvaGVh
cF9zdXJnZXJ5LmMKKysrIGIvY29udHJpYi9wZ19zdXJnZXJ5L2hlYXBfc3VyZ2VyeS5jCkBAIC0z
MTEsNyArMzExLDggQEAgaGVhcF9mb3JjZV9jb21tb24oRnVuY3Rpb25DYWxsSW5mbyBmY2luZm8s
IEhlYXBUdXBsZUZvcmNlT3B0aW9uIGhlYXBfZm9yY2Vfb3B0KQogCQkgKi8KIAkJaWYgKGRpZF9t
b2RpZnlfcGFnZSkKIAkJewotCQkJLyogTWFyayBidWZmZXIgZGlydHkgYmVmb3JlIHdlIHdyaXRl
IFdBTC4gKi8KKwkJCS8qIERlZnJhZ21lbnQgYW5kIG1hcmsgYnVmZmVyIGRpcnR5IGJlZm9yZSB3
ZSB3cml0ZSBXQUwuICovCisJCQlQYWdlUmVwYWlyRnJhZ21lbnRhdGlvbihwYWdlKTsKIAkJCU1h
cmtCdWZmZXJEaXJ0eShidWYpOwogCiAJCQkvKiBYTE9HIHN0dWZmICovCi0tIAoyLjMwLjIKCg==
--000000000000b883ee05d8681db8--