From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Robert Haas <robertmhaas@gmail.com>, Justin Pryzby <pryzby@telsasoft.com>,
        Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
        Erik Rijkers <er@xs4all.nl>,
        Matthias van de Meent <boekewurm+postgres@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
In-reply-to: <20220414165425.pxn22hdlwdrcouh6@alap3.anarazel.de>
References: <20220330.100617.1491348930290941094.horikyota.ntt@gmail.com> <20220330.114613.1177577626285014184.horikyota.ntt@gmail.com> <20220330.175824.936439875658680065.horikyota.ntt@gmail.com> <20220413202819.GR26620@telsasoft.com> <20220414003802.e37dgzkgbi5t7ha4@alap3.anarazel.de> <CA+TgmoZ1P5XxBvk1sU9s5hpHXAZf8u5oHTP1+h_otx2kHaG9QQ@mail.gmail.com> <1471620.1649947373@sss.pgh.pa.us> <CA+TgmoagKyvcMip+thF8JS0Jc9kAnQnZS0xBpTetixRukdh=qg@mail.gmail.com> <1484483.1649951206@sss.pgh.pa.us> <CA+TgmoaLb0j9zEfkk6bmuKM5422JenzzcJLJSiPLgwfMd+xnBw@mail.gmail.com> <20220414165425.pxn22hdlwdrcouh6@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Thu, 14 Apr 2022 09:54:25 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1533664.1649962046.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 14 Apr 2022 14:47:26 -0400
Message-ID: <1533665.1649962046@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/1533665.1649962046%40sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> On 2022-04-14 12:16:45 -0400, Robert Haas wrote:
>> I got curious and looked at the underlying problem here and I am
>> wondering whether HaveRegisteredOrActiveSnapshot() is just buggy. It
>> seems to me that the code is always going to return true if there are
>> any active snapshots, and the rest of the function is intended to test
>> whether there is a registered snapshot other than the catalog
>> snapshot. But I don't think that's what this code does:
>> =

>> if (pairingheap_is_empty(&RegisteredSnapshots) ||
>>     !pairingheap_is_singular(&RegisteredSnapshots))
>>      return false;
>> =

>> return CatalogSnapshot =3D=3D NULL;

> Certainly looks off...

Yeah, this is broken.  Whilst waiting around for a build on wrasse's
host, I reproduced the problem shown in this thread, and here's what
I see at the point of the exception:

(gdb) p RegisteredSnapshots
$5 =3D {ph_compare =3D 0x9a6000 <xmin_cmp>, ph_arg =3D 0x0, =

  ph_root =3D 0xec3168 <CatalogSnapshotData+72>}
(gdb) p *RegisteredSnapshots.ph_root
$6 =3D {first_child =3D 0x2d85d70, next_sibling =3D 0x0, prev_or_parent =3D=
 0x0}
(gdb) p CatalogSnapshotData
$7 =3D {snapshot_type =3D SNAPSHOT_MVCC, xmin =3D 52155, xmax =3D 52155, =

  xip =3D 0x2d855b0, xcnt =3D 0, subxip =3D 0x2de9130, subxcnt =3D 0, =

  suboverflowed =3D false, takenDuringRecovery =3D false, copied =3D false=
, =

  curcid =3D 0, speculativeToken =3D 0, vistest =3D 0x0, active_count =3D =
0, =

  regd_count =3D 0, ph_node =3D {first_child =3D 0x2d85d70, next_sibling =3D=
 0x0, =

    prev_or_parent =3D 0x0}, whenTaken =3D 0, lsn =3D 0, snapXactCompletio=
nCount =3D 1}
(gdb) p CatalogSnapshot    =

$8 =3D (Snapshot) 0xec3120 <CatalogSnapshotData>
(gdb) p *(Snapshot) (0x2d85d70-72)
$9 =3D {snapshot_type =3D SNAPSHOT_MVCC, xmin =3D 52155, xmax =3D 52155, x=
ip =3D 0x0, =

  xcnt =3D 0, subxip =3D 0x0, subxcnt =3D 0, suboverflowed =3D false, =

  takenDuringRecovery =3D false, copied =3D true, curcid =3D 0, =

  speculativeToken =3D 0, vistest =3D 0x0, active_count =3D 0, regd_count =
=3D 2, =

  ph_node =3D {first_child =3D 0x0, next_sibling =3D 0x0, =

    prev_or_parent =3D 0xec3168 <CatalogSnapshotData+72>}, whenTaken =3D 0=
, =

  lsn =3D 0, snapXactCompletionCount =3D 0}
(gdb) p ActiveSnapshot
$10 =3D (ActiveSnapshotElt *) 0x0

So in fact there IS another registered snapshot, and
HaveRegisteredOrActiveSnapshot is just lying.  I think the
correct test is more nearly what we have in
InvalidateCatalogSnapshotConditionally:

    if (CatalogSnapshot &&
        ActiveSnapshot =3D=3D NULL &&
        pairingheap_is_singular(&RegisteredSnapshots))
            // then the CatalogSnapshot is the only one.

Ergo, this actually is a bug in 277692220.

			regards, tom lane