public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andres Freund <[email protected]>
To: Robert Haas <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
Date: Thu, 14 Apr 2022 09:58:37 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CA+TgmoZ1P5XxBvk1sU9s5hpHXAZf8u5oHTP1+h_otx2kHaG9QQ@mail.gmail.com>
	<[email protected]>
	<CA+TgmoagKyvcMip+thF8JS0Jc9kAnQnZS0xBpTetixRukdh=qg@mail.gmail.com>
	<[email protected]>
	<CA+TgmoaLb0j9zEfkk6bmuKM5422JenzzcJLJSiPLgwfMd+xnBw@mail.gmail.com>
	<[email protected]>

Hi,

On 2022-04-14 09:54:25 -0700, Andres Freund wrote:
> 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 == NULL;
> 
> Certainly looks off...
> 
> 
> > I find that 'make check-world' passes with this change, which is
> > disturbing, because it also passes without this change. That means we
> > don't have any tests that reach HaveRegisteredOrActiveSnapshot() with
> > more than one registered snapshot.
> 
> Part of that is because right now the assertion is placed "too deep" -
> it should be much higher up, so it's reached even if there's not
> actually a toast datum. But there's of other bugs preventing that :(. A
> lot of bugs have been hidden by the existence of CatalogSnapshot (which
> of course isn't something one actually can rely on).
> 
> 
> > Also, unless we have plans to use HaveRegisteredOrActiveSnapshot() in
> > more places,
> 
> I think we should, but there's the other bugs that need to be fixed
> first :(. Namely that we have plenty places doing catalog accesses
> without an active or registered snapshot :(.

Ah, we actually were debating some of these issues more recently, in:
https://www.postgresql.org/message-id/20220311030721.olixpzcquqkw2qyt%40alap3.anarazel.de
https://www.postgresql.org/message-id/20220311021047.hgtqkrl6n52srvdu%40alap3.anarazel.de

It looks like the same bug, and that the patch in this thread fixes
them. And that we need to backpatch the fix, right?

Greetings,

Andres Freund

view thread (27+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox