Date: Sat, 19 Oct 2024 16:28:15 -0700
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	Robert Haas <robertmhaas@gmail.com>,
	=?utf-8?B?0JDQvdC00YDQtdC5INCg0LDRh9C40YbQutC40Lk=?= <therealgofman@mail.ru>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: BUG #18545: \dt breaks transaction, calling error when executed
 in SET SESSION AUTHORIZATION
Message-ID: <20241019232815.c6.nmisch@google.com>
References: <72802.1721415837@sss.pgh.pa.us>
 <1722241488.182053205@f169.i.mail.ru>
 <3593204.1722466756@sss.pgh.pa.us>
 <3618203.1722473994@sss.pgh.pa.us>
 <916377.1722809337@sss.pgh.pa.us>
 <CA+TgmoZ8owNGNkHUsoSL3cEd8C+zbb8w7-Oje0HYvfBp+btDBA@mail.gmail.com>
 <1807627.1722970465@sss.pgh.pa.us>
 <ZuoU_8EbSTE14o1U@pryzbyj2023>
 <1912093.1726716624@sss.pgh.pa.us>
 <2042942.1726781733@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="DkyZ4i47+24PcPiH"
Content-Disposition: inline
In-Reply-To: <2042942.1726781733@sss.pgh.pa.us>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20241019232815.c6.nmisch%40google.com>
Precedence: bulk


--DkyZ4i47+24PcPiH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Sep 19, 2024 at 05:35:33PM -0400, Tom Lane wrote:
> So the fix seems clear to me: RestoreRelationMap needs to happen
> before anything that could result in catalog lookups.  I'm kind
> of inclined to move up the adjacent restores of non-transactional
> low-level stuff too, particularly RestoreReindexState which has
> direct impact on how catalog lookups are done.

Thanks for debugging that.  RestorePendingSyncs() also changes what
RelationInitPhysicalAddr() puts in the relcache entry, so it needs to stay
with RestoreRelationMap().  I'm attaching the fix I have in mind.  Since the
above (commit 126ec0b), the following has been getting an
AssertPendingSyncConsistency() trap:

  make check-tests TESTS=reindex_catalog PG_TEST_INITDB_EXTRA_OPTS='-cwal_level=minimal -cmax_wal_senders=0'

In commit range [6e086fa,126ec0b^], that failed differently, reflecting the
outdated relmapper.  (6e086fa is mostly innocent here, though.)

I looked for ways we'd regret not back-patching commit 126ec0b and this, but I
didn't find a concrete problem.  After InvalidateSystemCaches(), the
back-branch parallel worker relcache contains the nailed relations.  Each
entry then has:

- rd_locator possibly outdated
- rd_firstRelfilelocatorSubid==0 (correct for rd_locator)
- rd_isvalid==false, from RelationReloadNailed()