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>
In-Reply-To: <CAH2-Wz=DHb2hXGqcvuVyy0qC88VcYoaNjy2FFtb-xjQMOZTQqw@mail.gmail.com>
From: Peter Geoghegan <pg@bowt.ie>
Date: Sat, 19 Feb 2022 15:08:41 -0800
Message-ID: <CAH2-WzkiB-qcsBmWrpzP0nxvrQExoUts1d7TYShg_DrkOHeg4Q@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="0000000000001b8c1a05d86716a9"
Archived-At: <https://www.postgresql.org/message-id/CAH2-WzkiB-qcsBmWrpzP0nxvrQExoUts1d7TYShg_DrkOHeg4Q%40mail.gmail.com>
Precedence: bulk

--0000000000001b8c1a05d86716a9
Content-Type: text/plain; charset="UTF-8"

On Fri, Feb 18, 2022 at 5:00 PM Peter Geoghegan <pg@bowt.ie> wrote:
> Another testing strategy occurs to me: we could stress-test the
> implementation by simulating an environment where the no-cleanup-lock
> path is hit an unusually large number of times, possibly a fixed
> percentage of the time (like 1%, 5%), say by making vacuumlazy.c's
> ConditionalLockBufferForCleanup() call return false randomly. Now that
> we have lazy_scan_noprune for the no-cleanup-lock path (which is as
> similar to the regular lazy_scan_prune path as possible), I wouldn't
> expect this ConditionalLockBufferForCleanup() testing gizmo to be too
> disruptive.

I tried this out, using the attached patch. It was quite interesting,
even when run against HEAD. I think that I might have found a bug on
HEAD, though I'm not really sure.

If you modify the patch to simulate conditions under which
ConditionalLockBufferForCleanup() fails about 2% of the time, you get
much better coverage of lazy_scan_noprune/heap_tuple_needs_freeze,
without it being so aggressive as to make "make check-world" fail --
which is exactly what I expected. If you are much more aggressive
about it, and make it 50% instead (which you can get just by using the
patch as written), then some tests will fail, mostly for reasons that
aren't surprising or interesting (e.g. plan changes). This is also
what I'd have guessed would happen.

However, it gets more interesting. One thing that I did not expect to
happen at all also happened (with the current 50% rate of simulated
ConditionalLockBufferForCleanup() failure from the patch): if I run
"make check" from the pg_surgery directory, then the Postgres backend
gets stuck in an infinite loop inside lazy_scan_prune, which has been
a symptom of several tricky bugs in the past year (not every time, but
usually). Specifically, the VACUUM statement launched by the SQL
command "vacuum freeze htab2;" from the file
contrib/pg_surgery/sql/heap_surgery.sql, at line 54 leads to this
misbehavior.

This is a temp table, which is a choice made by the tests specifically
because they need to "use a temp table so that vacuum behavior doesn't
depend on global xmin". This is convenient way of avoiding spurious
regression tests failures (e.g. from autoanalyze), and relies on the
GlobalVisTempRels behavior established by Andres' 2020 bugfix commit
94bc27b5.

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. At the very least, it wouldn't hurt to Assert() that the
target table isn't a temp table inside lazy_scan_noprune, documenting
our assumptions around temp tables and
ConditionalLockBufferForCleanup().

I haven't actually tried to debug the issue just yet, so take all this
with a grain of salt.

-- 
Peter Geoghegan

--0000000000001b8c1a05d86716a9
Content-Type: text/plain; charset="US-ASCII"; 
	name="0001-Add-adversarial-ConditionalLockBufferForCleanup-gizm.txt"
Content-Disposition: attachment; 
	filename="0001-Add-adversarial-ConditionalLockBufferForCleanup-gizm.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_kzugbj2e0>
X-Attachment-Id: f_kzugbj2e0

RnJvbSAzZjAxMjgxYWYzYmE4MWIzNTc3N2NiN2Q3MTdmNzZlMDAxZmQzZTEwIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBHZW9naGVnYW4gPHBnQGJvd3QuaWU+CkRhdGU6IFNh
dCwgMTkgRmViIDIwMjIgMTQ6MDc6MzUgLTA4MDAKU3ViamVjdDogW1BBVENIXSBBZGQgYWR2ZXJz
YXJpYWwgQ29uZGl0aW9uYWxMb2NrQnVmZmVyRm9yQ2xlYW51cCgpIGdpem1vIHRvCiB2YWN1dW1s
YXp5LmMuCgotLS0KIHNyYy9iYWNrZW5kL2FjY2Vzcy9oZWFwL3ZhY3V1bWxhenkuYyB8IDM2ICsr
KysrKysrKysrKysrKysrKysrKysrKysrKy0KIDEgZmlsZSBjaGFuZ2VkLCAzNSBpbnNlcnRpb25z
KCspLCAxIGRlbGV0aW9uKC0pCgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvYWNjZXNzL2hlYXAv
dmFjdXVtbGF6eS5jIGIvc3JjL2JhY2tlbmQvYWNjZXNzL2hlYXAvdmFjdXVtbGF6eS5jCmluZGV4
IDI0MjUxMWEyMy4uMzFjNmIzNjBlIDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9hY2Nlc3MvaGVh
cC92YWN1dW1sYXp5LmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL2hlYXAvdmFjdXVtbGF6eS5j
CkBAIC01MCw2ICs1MCw3IEBACiAjaW5jbHVkZSAiY29tbWFuZHMvZGJjb21tYW5kcy5oIgogI2lu
Y2x1ZGUgImNvbW1hbmRzL3Byb2dyZXNzLmgiCiAjaW5jbHVkZSAiY29tbWFuZHMvdmFjdXVtLmgi
CisjaW5jbHVkZSAiY29tbW9uL3BnX3BybmcuaCIKICNpbmNsdWRlICJleGVjdXRvci9pbnN0cnVt
ZW50LmgiCiAjaW5jbHVkZSAibWlzY2FkbWluLmgiCiAjaW5jbHVkZSAib3B0aW1pemVyL3BhdGhz
LmgiCkBAIC03NDgsNiArNzQ5LDM5IEBAIGhlYXBfdmFjdXVtX3JlbChSZWxhdGlvbiByZWwsIFZh
Y3V1bVBhcmFtcyAqcGFyYW1zLAogCX0KIH0KIAorLyoKKyAqIEFkdmVyc2FyaWFsIGdpem1vLCBz
aW11bGF0ZXMgZXhjZXNzaXZlIGZhaWx1cmUgdG8gZ2V0IGNsZWFudXAgbG9ja3MKKyAqLworc3Rh
dGljIGlubGluZSBib29sCitsYXp5X2NvbmRpdGlvbmFsbG9ja2J1ZmZlcmZvcmNsZWFudXAoQnVm
ZmVyIGJ1ZmZlcikKK3sKKwkvKgorCSAqIEFydGlmaWNpYWxseSBmYWlsIHRvIGdldCBhIGNsZWFu
dXAgbG9jayA1MCUgb2YgdGhlIHRpbWUuCisJICoKKwkgKiBYWFg6IFdoYXQgYWJvdXQgdGVtcCB0
YWJsZXM/ICBXZSBzaW11bGF0ZSBub3QgZ2V0dGluZyBhIGNsZWFudXAgbG9jaworCSAqIHRoZXJl
LCBidXQgaXMgdGhhdCBjaG9pY2UgYWN0dWFsbHkgcmVhc29uYWJsZT8KKwkgKi8KKwlpZiAocGdf
cHJuZ191aW50MzIoJnBnX2dsb2JhbF9wcm5nX3N0YXRlKSA8PSAoUEdfVUlOVDMyX01BWCAvIDIp
KQorCQlyZXR1cm4gZmFsc2U7CisKKyNpZiAwCisJLyoKKwkgKiA1MCUgaXMgdmVyeSB2ZXJ5IGFn
Z3Jlc3NpdmUsIHdoaWxlIDIlIC0gNSUgaXMgc3RpbGwgYmFzaWNhbGx5CisJICogYWR2ZXJzYXJp
YWwgYnV0IGluIG1hbnkgd2F5cyBsZXNzIGFubm95aW5nLgorCSAqCisJICogVGhpcyB2ZXJzaW9u
ICh3aGljaCBpbmplY3RzIGEgZmFpbHVyZSB0byBnZXQgYSBjbGVhbnVwIGxvY2sgMiUgb2YgdGhl
CisJICogdGltZSkgc2VlbXMgdG8gcGFzcyB0aGUgcmVncmVzc2lvbiB0ZXN0cywgZXZlbiB3aXRo
IG15IHBhcmFsbGVsIG1ha2UKKwkgKiBjaGVjay13b3JsZCByZWNpcGUuICBFeHBlY3RlZCBxdWVy
eSBwbGFucyBkb24ndCBzZWVtIHRvIHNoaWZ0IG9uCisJICogYWNjb3VudCBvZiB1bmV4cGVjdGVk
IGluZGV4IGJsb2F0IChub3IgYXJlIHRoZXJlIGFueSBwcm9ibGVtcyBvZiBhCisJICogc2ltaWxh
ciBuYXR1cmUpIHdpdGggdGhpcyB2YXJpYW50IG9mIHRoZSBnaXptby4KKwkgKi8KKwlpZiAocGdf
cHJuZ191aW50MzIoJnBnX2dsb2JhbF9wcm5nX3N0YXRlKSA8PSAoUEdfVUlOVDMyX01BWCAvIDUw
KSkKKwkJcmV0dXJuIGZhbHNlOworI2VuZGlmCisKKwlyZXR1cm4gQ29uZGl0aW9uYWxMb2NrQnVm
ZmVyRm9yQ2xlYW51cChidWZmZXIpOworfQorCiAvKgogICoJbGF6eV9zY2FuX2hlYXAoKSAtLSB3
b3JraG9yc2UgZnVuY3Rpb24gZm9yIFZBQ1VVTQogICoKQEAgLTEwOTMsNyArMTEyNyw3IEBAIGxh
enlfc2Nhbl9oZWFwKExWUmVsU3RhdGUgKnZhY3JlbCwgaW50IG53b3JrZXJzKQogCQkgKiBhIGNs
ZWFudXAgbG9jayByaWdodCBhd2F5LCB3ZSBtYXkgYmUgYWJsZSB0byBzZXR0bGUgZm9yIHJlZHVj
ZWQKIAkJICogcHJvY2Vzc2luZyB1c2luZyBsYXp5X3NjYW5fbm9wcnVuZS4KIAkJICovCi0JCWlm
ICghQ29uZGl0aW9uYWxMb2NrQnVmZmVyRm9yQ2xlYW51cChidWYpKQorCQlpZiAoIWxhenlfY29u
ZGl0aW9uYWxsb2NrYnVmZmVyZm9yY2xlYW51cChidWYpKQogCQl7CiAJCQlib29sCQloYXN0dXAs
CiAJCQkJCQlyZWNvcmRmcmVlc3BhY2U7Ci0tIAoyLjMwLjIKCg==
--0000000000001b8c1a05d86716a9--