Date: Thu, 5 Sep 2024 15:04:34 -0700
From: Noah Misch <noah@leadboat.com>
To: Nitin Motiani <nitinmotiani@google.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>,
	Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org,
	Tom Lane <tgl@sss.pgh.pa.us>, Smolkin Grigory <smallkeen@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	Alexander Lakhin <exclusion@gmail.com>
Subject: Re: race condition in pg_class
Message-ID: <20240905220434.80.nmisch@google.com>
References: <20240817040748.f8.nmisch@google.com>
 <63c27c17-0d8f-4ede-a624-3ee41d28b539@iki.fi>
 <20240822073200.4f.nmisch@google.com>
 <CAH5HC94OEarC9=FbS0o4D5DYKODMeYjVJ_KxxcBRQFMiMAOfZg@mail.gmail.com>
 <20240831011043.2b.nmisch@google.com>
 <CAH5HC96yPfrLVQ-srZ65R73t_dBLNM4mCR-RGcT5zNa04DHSxA@mail.gmail.com>
 <20240903212329.61.nmisch@google.com>
 <CAH5HC96ASNkXyYVEG7pO98qr1WARGTohxGjVpBsu4c3hkM_OcA@mail.gmail.com>
 <20240904195720.cf.nmisch@google.com>
 <CAH5HC96ZwrjKF-RBijntw9uX2UzUtEKC3A7cxB_Xh8arVuKhKw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAH5HC96ZwrjKF-RBijntw9uX2UzUtEKC3A7cxB_Xh8arVuKhKw@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240905220434.80.nmisch%40google.com>
Precedence: bulk

On Thu, Sep 05, 2024 at 07:10:04PM +0530, Nitin Motiani wrote:
> On Thu, Sep 5, 2024 at 1:27 AM Noah Misch <noah@leadboat.com> wrote:
> > On Wed, Sep 04, 2024 at 09:00:32PM +0530, Nitin Motiani wrote:
> > >  Assert(rel->ri_needsLockTagTuple ==  IsInplaceUpdateRelation(rel->relationDesc)
> > >
> > > This can safeguard against users of ResultRelInfo missing this field.
> >
> > v10 does the rename and adds that assertion.  This question remains open:
> 
> Looks good. A couple of minor comments :
> 1. In the inplace110 commit message, there are still references to
> heap_inplace_update. Should it be clarified that the function has been
> renamed?

PGXN has only one caller of this function, so I think that wouldn't help
readers enough.  If someone gets a compiler error about the old name, they'll
figure it out without commit log guidance.  If a person doesn't get a compiler
error, they didn't need to read about the fact of the rename.

> 2. Should there be a comment above the ri_needLockTag definition in
> execNodes.h that we are caching this value to avoid function calls to
> IsInPlaceUpdateRelation for every tuple? Similar to how the comment
> above ri_TrigFunctions mentions that it is cached lookup info.

Current comment:

	/* updates do LockTuple() before oldtup read; see README.tuplock */
	bool		ri_needLockTagTuple;

Once the comment doesn't fit in one line, pgindent rules make it take a
minimum of four lines.  I don't think words about avoiding function calls
would add enough value to justify the vertical space, because a person
starting to remove it would see where it's called.  That's not to say the
addition would be negligent.  If someone else were writing the patch and had
included that, I wouldn't be deleting the material.