Re: Consistently use the XLogRecPtrIsInvalid() macro

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.

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

> I mean, some people like writing if (!foo)
> and some like writing if (foo == NULL), but we're not going to legislate one
> over the other.

From that example, I understand that foo is a pointer.
I'am not sure that's a fair comparison with what this patch is doing. In your
example both are valids and not linked to any hardcoded value we set in the
code tree (as opposed to InvalidOid = 0).

> But we're certainly not going to introduce, uh, if
> (PointerIsValid(foo)), and in fact we just removed that!

I agree and I don't think the patch does that. It does not handle pointer implicit
comparisons (if it does in some places, that's a mistake).

What it does, is that when we have if (foo) and foo is an Oid (not a pointer to 
an Oid) then change to if (OidIsValid(foo)).

So, instead of treating Oid values as booleans, the patch makes use of OidIsValid(),
which makes the intent clear: I'm checking if this Oid is valid, not "I'm checking
if this integer is non zero."

> What you're
> proposing here seem quite analogous but in the opposite direction.
> 

I agree with the pointer case and I'll double check there is no such changes.

To clarify what the patches do, the transformations are (for var of type Oid):

- var != InvalidOid -> OidIsValid(var)
- var == InvalidOid -> !OidIsValid(var)
- var != 0 -> OidIsValid(var) (only when var is Oid, not pointer)
- var == 0 -> OidIsValid(var) (only when var is Oid, not pointer)
- var = 0 -> var = InvalidOid

The above is same idea as a2b02293bc6.

The one case I want to double check is transformations like:
  if (tab->newTableSpace) -> if (OidIsValid(tab->newTableSpace))

Here, tab is a pointer to a struct, but newTableSpace is an Oid field. The
original code treats the Oid as a boolean. This is not a pointer validity check,
it's checking if the Oid value is non zero.

Do you consider this acceptable?

Regards,

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