Message-ID: <24a26615b66d84abf2083a10ff1ca59933a7b4e2.camel@postgrespro.ru>
Subject: Re: can't drop table due to reference from orphaned temp function
From: Yura Sokolov <y.sokolov@postgrespro.ru>
To: Andres Freund <andres@anarazel.de>, Miles Delahunty
	 <miles.delahunty@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>
Date: Tue, 12 Jul 2022 17:13:28 +0300
In-Reply-To: <20220219213139.2gpsrqwp7hlmn634@alap3.anarazel.de>
References: 
	<CAOFAq3BU5Mf2TTvu8D9n_ZOoFAeQswuzk7yziAb7xuw_qyw5gw@mail.gmail.com>
	 <20220219180002.6tubjq7iw7m52bgd@alap3.anarazel.de>
	 <20220219213139.2gpsrqwp7hlmn634@alap3.anarazel.de>
Organization: PostgresPro
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/24a26615b66d84abf2083a10ff1ca59933a7b4e2.camel%40postgrespro.ru>
Precedence: bulk

Good day, Andres.

В Сб, 19/02/2022 в 13:31 -0800, Andres Freund пишет:
> Hi,
> 
> On 2022-02-19 10:00:02 -0800, Andres Freund wrote:
> > See backtrace below [1]. The same problem does *not* exist when starting to
> > use the same temp schema in a new session (which drops the old contents
> > first), which kind of explains why we've not previously noticed this.
> > 
> > But even so, I'm surprised we haven't noticed this before.
> 
> Ah, there's a reason for that. In many cases we'll have a catalog snapshot
> registered, which is enough for init_toast_snapshot(). But in Miles' example,
> the object dropped just prior ends with catalog invalidations.
> 
> Proposed bugfix and test attached.
> 
> I think it's ok to backpatch the test. There might be a slight change in
> output due to 618c16707a6d6e8f5c83ede2092975e4670201ad not being backpatched,
> but that's OK I think.
> 
> 
> I think it is dangerous that we return a cached catalog snapshot for things
> like GetOldestSnapshot() unless they're also registered or active - we can't
> rely on catalog snapshots to be present. Indeed, if we didn't, this bug would
> have been found before, as some added assertions confirm.
> 
> I don't think we can just ignore the catalog snapshot though, it can be
> registered in the future, so it actually is the oldest snapshot. But at least
> we should assert that there's some snapshot registered/active. In the attached
> patch I've added HaveRegisteredOrActiveSnapshot() and used that in
> init_toast_snapshot().

Reading your message and HaveRegisteredOrActiveSnapshot's body, I can't get its logic:
- if it is dangerous to have CatalogSnapshot alone in RegisteredSnapshots,
  then why we return 'false' if RegisteredSnapshots is NOT singular?

I believe, body of HaveRegisteredOrActiveSnapshot should look like:

	if (ActiveSnapshot != NULL)
		return true;

	if (pairingheap_is_empty(&RegisteredSnapshots))
	    return false;

	/*
	 * The catalog snapshot is in RegisteredSnapshots when valid, but can be
	 * removed at any time due to invalidation processing. If explicitly
	 * registered more than one snapshot has to be in RegisteredSnapshots.
	 */	
	if (pairingheap_is_singular(&RegisteredSnapshots))
		return CatalogSnapshot == NULL;
	
	return true;

Am I wrong?

regards,

Yura