Re: Consistently use the XLogRecPtrIsInvalid() macro - Mailing list pgsql-hackers

Hi,

On Tue, Nov 18, 2025 at 06:57:33PM +0100, Álvaro Herrera wrote:
> On 2025-Nov-18, Bertrand Drouvot wrote:
> > 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
>

Thanks for the info!

> +/*
> + * 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?

If it's not consistently used where it should be, and we're comparing against
InvalidOid instead, then I think that it looks useless. But if we ensure it's
used consistently throughout the codebase, then there is value in it.

Looking at 0001, it's not used for function calls in ginutil.c, not used
for boolean comparisons in plancat.c and selfuncs.c, and not used for variables
in regproc.c. I agree that the function calls and boolean comparisons changes
may look too noisy, but what about doing the regproc.c changes then and keep
RegProcedureIsValid()?

> > 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.

Right.

> We got rid of the comparisons with 0 and
> InvalidXLogRecPtr as a secondary effect, but that wasn't the main point.

I agree, but I think that was a good thing to do. I mean, ensuring the macro is
used sounds right. If not, what's the point of having the macro?

> 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.

Yeah, that's a lot of changes and maybe some of them are too noisy (like the ones
involving function calls). Maybe we could filter out what seems worth changing?

Keep those changes (for variables only):

- var != InvalidOid -> OidIsValid(var)
- var == InvalidOid -> !OidIsValid(var)

That would mean excluding the current 0 comparisons changes, but I'm not sure
that's fine, see [1] for other type example.

and maybe:

- var = 0 -> var = InvalidOid (when var is Oid)

I think that for variables only (i.e excluding function calls and implicit boolean
comparisons) that would be easier to read and less noisy. Worth a try to see what
it would look like?

[1]: https://www.postgresql.org/message-id/CACJufxGmsphWX150CxMU6KSed-x2fmTH3UpCwHpedGmjV1Biug%40mail.gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com