Обсуждение: Consistently use the XLogRecPtrIsInvalid() macro

Hi hackers,

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.

I think that's one of the things we could do once a year, like Peter does with
his annual "clang-tidy" check ([2]).

Thoughts?

[1]: https://www.postgresql.org/message-id/CAD21AoB_C6V1PLNs%3DSuOejgGh1o6ZHJMstD7X4X1b_z%3D%3DLdH1Q%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAH2-WzmxPQAF_ZhwrUo3rzVk3yYj_4mqbgiQXAGGO5nFYV3D8Q@mail.gmail.com

Regards,

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

On 10/28/25 4:13 PM, Bertrand Drouvot wrote:
> Hi hackers,
> 
> 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.

> I think that's one of the things we could do once a year, like Peter does with
> his annual "clang-tidy" check ([2]).
> 
> Thoughts?
> 
> [1]:
https://www.postgresql.org/message-id/CAD21AoB_C6V1PLNs%3DSuOejgGh1o6ZHJMstD7X4X1b_z%3D%3DLdH1Q%40mail.gmail.com
> [2]: https://www.postgresql.org/message-id/CAH2-WzmxPQAF_ZhwrUo3rzVk3yYj_4mqbgiQXAGGO5nFYV3D8Q@mail.gmail.com
> 
> Regards,
> 

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

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.

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'

- Heikki

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

On Tue, Oct 28, 2025 at 01:40:24PM +0200, Heikki Linnakangas wrote:
> 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.
>
> 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'

The annoying part with eliminating XLogRecPtrIsInvalid() or replacing
it is that a bunch of external code would be broken, particularly
backup tools.  I'd rather leave the beast alone.
--
Michael

On 2025-Oct-28, Michael Paquier wrote:

> The annoying part with eliminating XLogRecPtrIsInvalid() or replacing
> it is that a bunch of external code would be broken, particularly
> backup tools.  I'd rather leave the beast alone.

Well, we don't have to remove it right away.  We can simply not use it.
And maybe if the compiler supports it, make a static inline function in
the header with the [[deprecated]] attribute or
__attribute__((deprecated)) so that the tool developers get a warning
and migrate to using the new one.  Then in a few years we remove it.

BTW we could use Coccinelle to replace all the XLogRecPtrIsInvalid()
calls with !XLogRecPtrIsValid(), as well as all places comparing an LSN
to InvalidXLogRecPtr or literal zero.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Hi,

On Tue, Oct 28, 2025 at 04:05:34PM +0200, Álvaro Herrera wrote:
> 
> BTW we could use Coccinelle to replace all the XLogRecPtrIsInvalid()
> calls with !XLogRecPtrIsValid(), as well as all places comparing an LSN
> to InvalidXLogRecPtr or literal zero.

I did v1 the old way (shell script) and did not think about using
Coccinelle for this. That's a good idea and well suited for this purpose: I'll
work on it. Thanks for the suggestion!

Regards,

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

On 28.10.25 13:33, Bertrand Drouvot wrote:
> 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.

If we wanted real type safety, we could turn XLogRecPtr back into a 
struct, and then enforce the use of XLogRecPtrIsValid() and similar. 
Otherwise, we should just acknowledge that it's an integer and use 
integer code to deal with it.  These *IsValid() and similar macros that 
are there for "readability" but are not actually enforced other than by 
some developers' willpower are just causing more work and inconsistency 
in the long run.

Hi,

On Tue, Oct 28, 2025 at 05:57:54PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Tue, Oct 28, 2025 at 04:05:34PM +0200, Álvaro Herrera wrote:
> > 
> > BTW we could use Coccinelle to replace all the XLogRecPtrIsInvalid()
> > calls with !XLogRecPtrIsValid(), as well as all places comparing an LSN
> > to InvalidXLogRecPtr or literal zero.
> 
> I did v1 the old way (shell script) and did not think about using
> Coccinelle for this. That's a good idea and well suited for this purpose: I'll
> work on it. Thanks for the suggestion!

PFA a series of patches to implement what has been discussed above i.e:

- Introduce XLogRecPtrIsValid() and replace XLogRecPtrIsInvalid() calls: this
is done in 0001. The replacement changes have been generated by the Coccinelle
script [1].

- 0002 deprecates XLogRecPtrIsInvalid(): it emits a warning message at compilation
time if XLogRecPtrIsInvalid() is in use in the code base.

- 0003 replaces InvalidXLogRecPtr comparisons with XLogRecPtrIsValid(). The
replacement changes have been generated by the Coccinelle script [2].

- 0004 replaces literal 0 comparisons on XLogRecPtr with XLogRecPtrIsValid(). The
replacement changes have been generated by the Coccinelle script [3].

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_XLogRecPtrIsInvalid.cocci 
[2]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_InvalidXLogRecPtr_comparisons.cocci
[3]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_literal_0_comparisons.cocci

Regards,

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

Hi,

On Wed, Oct 29, 2025 at 05:50:13PM +0100, Peter Eisentraut wrote:
> On 28.10.25 13:33, Bertrand Drouvot wrote:
> > 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.
> 
> If we wanted real type safety, we could turn XLogRecPtr back into a struct,
> and then enforce the use of XLogRecPtrIsValid() and similar.

Right. That said I think that we'd need an opaque struct to avoid developers
doing things like: lsn.value == InvalidXLogRecPtr. If not, we'd still
need regular checks to ensure the macro is used. Opaque struct would probably
add extra costs too.

> Otherwise, we
> should just acknowledge that it's an integer and use integer code to deal
> with it.  These *IsValid() and similar macros that are there for
> "readability" but are not actually enforced other than by some developers'
> willpower are just causing more work and inconsistency in the long run.

That's a good point. Scripts (like the ones shared in [1]) can catch violations,
but it's still "manual" enforcement.

We don't currently enforce the other *IsValid() macros. I think it would be worth
setting up checks for all of them, but I agree that's new ongoing maintenance
work.

Regards,

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

On 30.10.25 10:17, Bertrand Drouvot wrote:
> - 0002 deprecates XLogRecPtrIsInvalid(): it emits a warning message at compilation
> time if XLogRecPtrIsInvalid() is in use in the code base.

Surely this could be factored out in macro in such a way that the 
warning message is a macro argument and we could reuse this attribute 
elsewhere in the code.

That said, I'm suspicious about marking things deprecated before the 
replacement is widely available.  If an extension has been using 
XLogRecPtrIsInvalid() until now (which has been best practice), when 
that extension adds PG19 support, they will either have to backport 
XLogRecPtrIsValid or turn off deprecation warnings.  At least there 
should be some guidance about what you expect third-party code to do 
about this.

Hi,

On Thu, Oct 30, 2025 at 02:55:51PM +0100, Peter Eisentraut wrote:
> On 30.10.25 10:17, Bertrand Drouvot wrote:
> > - 0002 deprecates XLogRecPtrIsInvalid(): it emits a warning message at compilation
> > time if XLogRecPtrIsInvalid() is in use in the code base.
> 
> Surely this could be factored out in macro in such a way that the warning
> message is a macro argument and we could reuse this attribute elsewhere in
> the code.

Yeah, I thought about it and initially considered waiting until we have another
use case. But you're right that it's better to do it from the start.

> That said, I'm suspicious about marking things deprecated before the
> replacement is widely available.  If an extension has been using
> XLogRecPtrIsInvalid() until now (which has been best practice), when that
> extension adds PG19 support, they will either have to backport
> XLogRecPtrIsValid or turn off deprecation warnings.

That's a good point, thanks! I agree with your concern.

After giving it more thought, I'm inclined to postpone the compiler warning
until XLogRecPtrIsValid() has been available for some time. The question is for
how long?

I did some research, reading [1] (but that's not really identical) or looking
at what we did for example for "SPI_prepare_cursor", "SPI_prepare_params"
and friends (but again that's not really identical): 844fe9f159a mentioned
that they "can perhaps go away at some point" and are documented as
deprecated since PG14.

So, back at our case, a conservative approach would be to wait for 5
years until the new macro is available on all the supported major versions.
That would mean introduce the deprecated attribute in PG24.

I don't see waiting for PG24 as an issue: the main goal of the new macro is
to be consistent with other *IsValid() ones and avoid "double negation" and that
would be achieved in the core code base.

For PG19, we could:

Add a comment in the code documenting that XLogRecPtrIsInvalid() is deprecated
and that we will enforce a "deprecated" attribute on it in PG24.

> At least there should
> be some guidance about what you expect third-party code to do about this.

The alternative of adding the warning immediately with migration guidance would
still create "friction". Especially if we deprecate multiple APIs following this
new pattern (i.e adding a "deprecated" attribute). As you said, they could
backport the macro themselves but that seems like unnecessary friction when we
can simply wait.

What do you think?

[1]: https://www.postgresql.org/message-id/4992828D.8000307%40gmx.net

Regards,

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

On 31.10.25 05:31, Bertrand Drouvot wrote:
> For PG19, we could:
> 
> Add a comment in the code documenting that XLogRecPtrIsInvalid() is deprecated
> and that we will enforce a "deprecated" attribute on it in PG24.

Just a code comment for now seems reasonable.  I wouldn't make any 
predictions about the future in code comments, though.

Hi,

On Fri, Oct 31, 2025 at 08:41:46AM +0100, Peter Eisentraut wrote:
> On 31.10.25 05:31, Bertrand Drouvot wrote:
> > For PG19, we could:
> > 
> > Add a comment in the code documenting that XLogRecPtrIsInvalid() is deprecated
> > and that we will enforce a "deprecated" attribute on it in PG24.
> 
> Just a code comment for now seems reasonable.  I wouldn't make any
> predictions about the future in code comments, though.

Done that way in 0001 attached.

Also in passing I realized that v2 did miss doing a replacement:

"PageGetLSN(page) == InvalidXLogRecPtr" in vacuumlazy.c.

I changed it "manually" for now in 0002 and checked that no other replacements
is missing.

Will fix the associated coccinelle script.

Regards,

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

On 2025-Oct-31, Bertrand Drouvot wrote:

> After giving it more thought, I'm inclined to postpone the compiler warning
> until XLogRecPtrIsValid() has been available for some time. The question is for
> how long?

Maybe we can mark it so that it becomes obsolete in a future version,

#if PG_VERSION_NUM >= 210000 
[[obsolete]]
#endif
XLogRecPtrIsInvalid( .. )

so that people using it today won't get any noise, but once they do get
the warning, the versions without the other macro are already out of
support, so they can switch to the new one easily.  (This presupposes
that we'd add the new macro to older branches as well, which shouldn't
be a problem.)  Only extensions wishing to support PG versions older
than we support would have to work slightly harder, but that should be OK.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)

Hi,

On Fri, Oct 31, 2025 at 01:19:50PM +0100, Álvaro Herrera wrote:
> On 2025-Oct-31, Bertrand Drouvot wrote:
> 
> > After giving it more thought, I'm inclined to postpone the compiler warning
> > until XLogRecPtrIsValid() has been available for some time. The question is for
> > how long?
> 
> Maybe we can mark it so that it becomes obsolete in a future version,
> 
> #if PG_VERSION_NUM >= 210000 
> [[obsolete]]
> #endif
> XLogRecPtrIsInvalid( .. )
> 
> so that people using it today won't get any noise, but once they do get
> the warning, the versions without the other macro are already out of
> support, so they can switch to the new one easily.  (This presupposes
> that we'd add the new macro to older branches as well, which shouldn't
> be a problem.)  Only extensions wishing to support PG versions older
> than we support would have to work slightly harder, but that should be OK.

Yeah, I did not think of checking PG_VERSION_NUM for a "future" version, that's
a good idea! I did it that way in the attached (in 0002) and introduced
PG_DEPRECATED() (as suggested by Peter upthread).

The version check is done on 24 to ensure that the new macro is available on all
the supported major versions.

The PG_DEPRECATED() name and location (c.h) look fine to me but maybe there is
better suggestions.

I think the way it is done in the attached makes sense, it:

- introduces PG_DEPRECATED()
- provides a use case on how to use it (i.e using a version that is currently
in the future)
- ensures that XLogRecPtrIsInvalid() deprecation is "enforced" as of version 24
- ensures that not using XLogRecPtrIsInvalid() is documented (so that it should
not be used anymore, at least in core, even waiting for version 24)

Regards,

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