Feedback-ID: ie3de48e3:Fastmail
Date: Tue, 18 Nov 2025 18:57:33 +0100
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Peter Eisentraut <peter@eisentraut.org>,
	Michael Paquier <michael@paquier.xyz>, Heikki Linnakangas <hlinnaka@iki.fi>,
	Quan Zongliang <quanzongliang@yeah.net>, pgsql-hackers@lists.postgresql.org
Subject: Re: Consistently use the XLogRecPtrIsInvalid() macro
Message-ID: <202511181738.yokxzijfdfx7@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <aRypWtxoKm+Q49kx@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: 
 <https://www.postgresql.org/message-id/202511181738.yokxzijfdfx7%40alvherre.pgsql>
Precedence: bulk

On 2025-Nov-18, Bertrand Drouvot wrote:

> Hi,
> 
> On Tue, Nov 18, 2025 at 04:54:32PM +0100, Peter Eisentraut wrote:
> > RegProcedureIsValid() doesn't add any value over OidIsValid, and we
> > don't have any RegXXXIsValid() for any of the other regxxx types.
> > So if we were to do anything about this, I would just remove it.
> 
> The patch makes use of it because it exists. I agree that we could
> also remove it (for the reasons you mentioned above), I'll do that.

RegProcedure actually predates all of our reg* types -- it goes back to
the initial commit of Postgres in 1989, according to
https://github.com/kelvich/postgres_pre95/commit/c4af0cb9d2a391815eb513416d95d49760202a42#diff-843ff64714a2c04906cdc890ccf6aedd0444e395e4ec412e70465638e80b2011R182

+/*
+ * RegProcedureIsValid
+ */
+bool
+RegProcedureIsValid(procedure)
+   RegProcedure    procedure;
+{
+   return (ObjectIdIsValid(procedure));
+}

I'm not sure what to think now about this whole idea of removing it.
Maybe there's some documentation value in it -- that line is saying,
this is not just any Oid, it's the Oid of a procedure.  Is this
completely useless?

> > For OidIsValid etc., I don't think this improves the notation.  It
> > is well understood that InvalidOid is 0.
> 
> I agree and that's perfectly valid to use 0 (unless InvalidOid does not mean
> 0 anymore in the future for whatever reasons). That said I think that using
> the macro makes the code more consistent (same spirit as a2b02293bc6).

Well, what we were trying to do there was get rid of
XLogRecPtrIsInvalid(), which was clearly going against the grain re.
other IsValid macros.  We got rid of the comparisons with 0 and
InvalidXLogRecPtr as a secondary effect, but that wasn't the main point.
This new patch series is much noisier and it's not at all clear to me
that we're achieving anything very useful.  In fact, looking at proposed
changes, I would argue that there are several places that end up worse.

Honestly I'd leave this alone.  

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Ellos andaban todos desnudos como su madre los parió, y también las mujeres,
aunque no vi más que una, harto moza, y todos los que yo vi eran todos
mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)