Message-ID: <baa25a6fc7c725d28ec5d0b7787230180b45a275.camel@j-davis.com>
Subject: Re: pgsql: Avoid improbable PANIC during heap_update.
From: Jeff Davis <pgsql@j-davis.com>
To: Peter Geoghegan <pg@bowt.ie>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jaime Casanova <jcasanov@systemguards.com.ec>,
	pgsql-committers@lists.postgresql.org
Date: Sat, 01 Oct 2022 09:35:31 -0700
In-Reply-To: 
 <CAH2-WzkGUXW5+7Ry6r3iVN2vhU0cO74YrH07cXW8ykxjZUeWMA@mail.gmail.com>
References: <E1lWLjP-00006Y-Ml@gemulon.postgresql.org>
	 <YzVPfCKNrmi0aLEh@ahch-to> <YzdVJnQocQ6RHKxk@ahch-to>
	 <692791.1664573331@sss.pgh.pa.us>
	 <CAH2-Wz=Apna8uQAxdBUTTZX10baV-MsT9FyV6o-ifZD+knwVEQ@mail.gmail.com>
	 <741997.1664574971@sss.pgh.pa.us>
	 <CAH2-Wzmb3ENhfaLVv=+TwQyHSbUxvhsOPWo7Lsw7o-1kUbpzXw@mail.gmail.com>
	 <841352.1664581964@sss.pgh.pa.us>
	 <CAH2-WzmpfcbFxb=CNC4JiCzLLCGrWyWSV2kxaXVew9owKk4nrA@mail.gmail.com>
	 <CAH2-WznFSJs7UcO1yhTWiQHjYh4PYJULc3UXz+uftLVSNcUh-Q@mail.gmail.com>
	 <CAH2-WzkGUXW5+7Ry6r3iVN2vhU0cO74YrH07cXW8ykxjZUeWMA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.1-0ubuntu1 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/baa25a6fc7c725d28ec5d0b7787230180b45a275.camel%40j-davis.com>
Precedence: bulk

On Fri, 2022-09-30 at 21:23 -0700, Peter Geoghegan wrote:
> 2490=C2=A0=C2=A0=C2=A0=C2=A0 page =3D BufferGetPage(buffer);
> 2491
> 2492=C2=A0=C2=A0=C2=A0=C2=A0 /*
> 2493=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Before locking the buffer, pin the v=
isibility map page if
> it appears to
> 2494=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * be necessary.=C2=A0 Since we haven't=
 got the lock yet, someone
> else might be
> 2495=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * in the middle of changing this, so w=
e'll need to recheck
> after we have
> 2496=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * the lock.
> 2497=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
> 2498=C2=A0=C2=A0=C2=A0=C2=A0 if (PageIsAllVisible(page))
> 2499=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 visibilitymap_pin(re=
lation, block, &vmbuffer);
>=20
> So we're calling visibilitymap_pin() having just acquired a buffer
> pin
> on a heap page buffer for the first time, and without acquiring a
> buffer lock on the same heap page (we don't hold one now, and we've
> never held one at some earlier point).
>=20
> Without Jeff's bugfix, nothing stops heap_delete() from getting a pin
> on a heap page that happens to have already been cleanup locked by
> another session running VACUUM. The same session could then
> (correctly) observe that the page does not have PD_ALL_VISIBLE set --
> not yet.=C2=A0

With you so far; I had considered this code path and was still unable
to repro.

> VACUUM might then set PD_ALL_VISIBLE, without having to
> acquire any kind of interlock that heap_delete() will reliably
> notice.
> After all, VACUUM had a cleanup lock before the other session's call
> to heap_delete() even began -- so the responsibility has to lie with
> heap_delete().

Directly after the code you reference above, there is (in 5f9dda4c06,
right before my patch):

 2501     LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
 2502=20
 2503     /*                                                         =20
 2504      * If we didn't pin the visibility map page and the page has
become all                      =20
 2505      * visible while we were busy locking the buffer, we'll have
to unlock and                   =20
 2506      * re-lock, to avoid holding the buffer lock across an I/O.=20
That's a bit                    =20
 2507      * unfortunate, but hopefully shouldn't happen often.      =20
 2508      */
 2509     if (vmbuffer =3D=3D InvalidBuffer && PageIsAllVisible(page))
 2510     {
 2511         LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 2512         visibilitymap_pin(relation, block, &vmbuffer);
 2513         LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
 2514     }

Doesn't that deal with the case you brought up directly? heap_delete()
can't get the exclusive lock until VACUUM releases its cleanup lock, at
which point all-visible will be set. Then heap_delete() should notice
in line 2509 and then pin the VM buffer. Right?

And I don't think a similar issue exists when the locks are briefly
released on lines 2563 or 2606. The pin is held until after the VM bit
is cleared (aside from an error path and an early return):

 2489     buffer =3D ReadBuffer(relation, block);
 ...
 2717     if (PageIsAllVisible(page))
 2718     {
 2719         all_visible_cleared =3D true;
 2720         PageClearAllVisible(page);
 ...
 2835     ReleaseBuffer(buffer);

If VACCUM hasn't acquired the cleanup lock before 2489, it can't get
one until heap_delete() is done looking at the all-visible bit anyway.
So I don't see how my patch would have fixed it even if that was the
problem.

Of course, I must be wrong somewhere, because the bug seems to exist.

--=20
Jeff Davis
PostgreSQL Contributor Team - AWS