Re: Consistently use the XLogRecPtrIsInvalid() macro

Hi,

On Tue, Oct 28, 2025 at 01:40:24PM +0200, Heikki Linnakangas wrote:
> On 28/10/2025 10:53, Quan Zongliang wrote:
> > On 10/28/25 4:13 PM, Bertrand Drouvot wrote:
> > > While working on refactoring some code in [1], one of the changes was:
> > > 
> > > -       if (initial_restart_lsn != InvalidXLogRecPtr &&
> > > -           initial_restart_lsn < oldestLSN)
> > > +       XLogRecPtr  restart_lsn = s->data.restart_lsn;
> > > +
> > > +       if (restart_lsn != InvalidXLogRecPtr &&
> > > +           restart_lsn < oldestLSN)
> > > 
> > > Sawada-san suggested to use the XLogRecPtrIsInvalid() macro here.
> > > 
> > > But this != InvalidXLogRecPtr check was existing code, so why not
> > > consistently use XLogRecPtrIsInvalid() where we check equality
> > > against InvalidXLogRecPtr?
> > > 
> > > At the time the current XLogRecPtrIsInvalid() has been introduced
> > > (0ab9d1c4b316) all the InvalidXLogRecPtr equality checks were done
> > > using XLogRecPtrIsInvalid().
> > > 
> > > But since, it has changed: I looked at it and this is not the case
> > > anymore in 20 files.
> > > 
> > > PFA, patches to $SUBJECT.  To ease the review, I created one patch
> > > per modified file.
> > > 
> > > I suspect the same approach could be applied to some other macros
> > > too.  Let's start with XLogRecPtrIsInvalid() first.
> > 
> > Agree. This patch has made the code style more consistent. And using
> > such macros is also in line with the usual practice. Just like
> > OidIsValid and TransactionIdIsValid.
> 
> Back in the day, XLogRecPtrIsInvalid() was needed because XLogRecPtr was a
> struct. 'x == InvalidXLogRecPtr' simply did not work.

Thank you both for your feedback!

> I don't see much need
> for it nowadays. In fact I wonder if we should go in the other direction and
> replace XLogRecPtrIsInvalid(x) calls with 'x == InvalidXLogRecPtr'.

That would be an option to ensure code consistency (related tp XLogRecPtr checks)
that will be hard to break (unless someone re-introduces a similar macro).

But OTOH that would introduce some kind of inconsistency with OidIsValid() and
TransactionIdIsValid() for example.

> It's also a bit cumbersome that we have XLogRecPtrIsInvalid() rather than
> XLogRecPtrIsValid(). That's inconsistent with OidIsValid and
> TransactionIdInValid, and it leads to an awkward double negative 'if
> (!XLogRecPtrIsInvalid(x))' if you want to check that 'x' is valid.

Agree.

> Overall I'm inclined to do nothing. But if anything, perhaps introduce
> XLogRecPtrIsValid(x) and switch to that, or replace XLogRecPtrIsInvalid(x)
> calls with 'x == InvalidXLogRecPtr'

I do prefer to introduce XLogRecPtrIsValid(x) and switch to that. Then, do the
same kind of work on OidIsValid() and TransactionIdIsValid() and add an annual
check.

Idea is to get some code consistency while keeping macros which are valuable for
readability and centralize changes if any need to be done in the way we check
their validity.

Regards,

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