Date: Sat, 19 Feb 2022 16:12:31 -0800
From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@lists.postgresql.org, Peter Geoghegan <pg@bowt.ie>
CC: Robert Haas <robertmhaas@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: =?US-ASCII?Q?Re=3A_Removing_more_vacuumlazy=2Ec_speci?= =?US-ASCII?Q?al_cases=2C_relfrozenxid_optimizations?=
User-Agent: K-9 Mail for Android
In-Reply-To: <CAH2-WzkiB-qcsBmWrpzP0nxvrQExoUts1d7TYShg_DrkOHeg4Q@mail.gmail.com>
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>
Message-ID: <87697D0B-3A41-4A5B-B9CF-85D989361E27@anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/87697D0B-3A41-4A5B-B9CF-85D989361E27%40anarazel.de>
Precedence: bulk

Hi,

(On phone, so crappy formatting and no source access)

On February 19, 2022 3:08:41 PM PST, Peter Geoghegan <pg@bowt=2Eie> wrote:
>On Fri, Feb 18, 2022 at 5:00 PM Peter Geoghegan <pg@bowt=2Eie> 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=2Ec's
>> ConditionalLockBufferForCleanup() call return false randomly=2E Now tha=
t
>> 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=2E
>
>I tried this out, using the attached patch=2E It was quite interesting,
>even when run against HEAD=2E I think that I might have found a bug on
>HEAD, though I'm not really sure=2E
>
>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=2E 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=2Eg=2E plan changes)=2E This is also
>what I'd have guessed would happen=2E
>
>However, it gets more interesting=2E 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)=2E Specifically, the VACUUM statement launched by the SQL
>command "vacuum freeze htab2;" from the file
>contrib/pg_surgery/sql/heap_surgery=2Esql, at line 54 leads to this
>misbehavior=2E


>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"=2E This is convenient way of avoiding spurious
>regression tests failures (e=2Eg=2E from autoanalyze), and relies on the
>GlobalVisTempRels behavior established by Andres' 2020 bugfix commit
>94bc27b5=2E

We don't have a blocking path for cleanup locks of temporary buffers IIRC =
(normally not reachable)=2E So I wouldn't be surprised if a cleanup lock fa=
iling would cause some odd behavior=2E

>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)=2E Even if the
>behavior that I saw does not indicate a bug on HEAD, it still seems
>informative=2E 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()=2E

Definitely worth looking into more=2E


This reminds me of a recent thing I noticed in the aio patch=2E Spgist can=
 end up busy looping when buffers are locked, instead of blocking=2E Not ac=
tually related, of course=2E

Andres
--=20
Sent from my Android device with K-9 Mail=2E Please excuse my brevity=2E