Date: Wed, 6 Sep 2023 15:13:20 -0400
From: Bruce Momjian <bruce@momjian.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
	Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?= <ilmari@ilmari.org>,
	pgsql-hackers@postgresql.org
Subject: Re: Obsolete reference to pg_relation in comment
Message-ID: <ZPjPUJBj+SVCPFfN@momjian.us>
References: <87sf9apnr0.fsf@wibble.ilmari.org>
 <20230726205031.GD3310393@nathanxps13>
 <1948637.1690406048@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="DYAIXimXmxO68xkL"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1948637.1690406048@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/ZPjPUJBj%2BSVCPFfN%40momjian.us>
Precedence: bulk


--DYAIXimXmxO68xkL
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Wed, Jul 26, 2023 at 05:14:08PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
> > On Wed, Jul 26, 2023 at 06:48:51PM +0100, Dagfinn Ilmari Mannsåker wrote:
> >>   * All accesses to pg_largeobject and its index make use of a single Relation
> >> - * reference, so that we only need to open pg_relation once per transaction.
> >> + * reference, so that we only need to open pg_class once per transaction.
> >>   * To avoid problems when the first such reference occurs inside a
> >>   * subtransaction, we execute a slightly klugy maneuver to assign ownership of
> >>   * the Relation reference to TopTransactionResourceOwner.
> 
> > Hm.  Are you sure this is actually referring to pg_class?  It seems
> > unlikely given pg_relation was renamed 14 years before this comment was
> > added, and the code appears to be ensuring that pg_largeobject and its
> > index are opened at most once per transaction.
> 
> I believe it is just a typo/thinko for pg_class, but there's more not
> to like about this comment.  First, once we've made a relcache entry
> it would typically stay valid across uses, so it's far from clear that
> this coding actually prevents many catalog accesses in typical cases.
> Second, when we do have to rebuild the relcache entry, there's a lot
> more involved than just a pg_class fetch; we at least need to read
> pg_attribute, and I think there may be other catalogs that we'd read
> along the way, even for a system catalog that lacks complicated
> features.  (pg_index would presumably get looked at, for instance.)
> 
> I think we should reword this to just generically claim that holding
> the Relation reference open for the whole transaction reduces overhead.

How is this attached patch?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

--DYAIXimXmxO68xkL
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="pg_relation.diff"

diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c
new file mode 100644
index 84e543e..cc9c335
*** a/src/backend/storage/large_object/inv_api.c
--- b/src/backend/storage/large_object/inv_api.c
***************
*** 58,68 ****
  bool		lo_compat_privileges;
  
  /*
!  * All accesses to pg_largeobject and its index make use of a single Relation
!  * reference, so that we only need to open pg_relation once per transaction.
!  * To avoid problems when the first such reference occurs inside a
!  * subtransaction, we execute a slightly klugy maneuver to assign ownership of
!  * the Relation reference to TopTransactionResourceOwner.
   */
  static Relation lo_heap_r = NULL;
  static Relation lo_index_r = NULL;
--- 58,68 ----
  bool		lo_compat_privileges;
  
  /*
!  * All accesses to pg_largeobject and its index make use of a single
!  * Relation reference.  To guarantee that the relcache entry remains
!  * in the cache, on the first reference inside a subtransaction, we
!  * execute a slightly klugy maneuver to assign ownership of the
!  * Relation reference to TopTransactionResourceOwner.
   */
  static Relation lo_heap_r = NULL;
  static Relation lo_index_r = NULL;

--DYAIXimXmxO68xkL--