Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Replace literal 0 with InvalidXLogRecPtr for XLogRecPtr assignments
- ec3174407164 19 (unreleased) landed
Replace pointer comparisons and assignments to literal zero with NULL
- ec782f56b0c3 19 (unreleased) landed
Use XLogRecPtrIsValid() in various places
- a2b02293bc65 19 (unreleased) landed
Introduce XLogRecPtrIsValid()
- d2965f627fe3 14.20 landed
- c0031d461324 18.1 landed
- 723cc84db50a 16.11 landed
- 49b45999f3b2 15.15 landed
- 33727aff18d0 17.7 landed
- 20bafb097288 13.23 landed
- 06edbed47862 19 (unreleased) landed

Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-10-28T08:13:06Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Quan Zongliang <quanzongliang@yeah.net> — 2025-10-28T08:53:29Z


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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Heikki Linnakangas <hlinnaka@iki.fi> — 2025-10-28T11:40:24Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-10-28T12:33:05Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Michael Paquier <michael@paquier.xyz> — 2025-10-28T13:10:08Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-10-28T14:05:34Z

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/

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-10-28T17:57:54Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Peter Eisentraut <peter@eisentraut.org> — 2025-10-29T16:50:13Z

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.

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-10-30T09:17:59Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-10-30T09:20:02Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Peter Eisentraut <peter@eisentraut.org> — 2025-10-30T13:55:51Z

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.

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-10-31T04:31:41Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Peter Eisentraut <peter@eisentraut.org> — 2025-10-31T07:41:46Z

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.

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-10-31T09:53:53Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-10-31T12:19:50Z

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)

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-03T07:47:28Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-06T05:56:47Z

Hi,

On Mon, Nov 03, 2025 at 07:47:28AM +0000, Bertrand Drouvot wrote:
> 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)

Attached a rebase due to 6d0eba66275 and 447aae13b03.

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-06T09:06:13Z

On 2025-Nov-06, Bertrand Drouvot wrote:

> Subject: [PATCH v5 1/4] Introduce XLogRecPtrIsValid() and replace
>  XLogRecPtrIsInvalid() calls

> XLogRecPtrIsInvalid() is inconsistent with the affirmative form of other
> *IsValid() macros and leads to awkward double negative.
> 
> This commit introduces XLogRecPtrIsValid() and replace all the
> XLogRecPtrIsInvalid() calls.
> 
> It also adds a comment mentioning that new code should use XLogRecPtrIsValid()
> instead of XLogRecPtrIsInvalid() and that XLogRecPtrIsInvalid() could be
> deprecated in the future.

I think we should do this in two steps.  First, introduce
XLogRecPtrIsValid(), don't use it anywhere, backpatch this one.  This
would alleviate potential backpatching pains when using the new macro in
future bugfixes.  Second, change calls of the old function to the new
one, no backpatch.

> From 22f02ca0618d9f2e34de8fa084127bf500d75603 Mon Sep 17 00:00:00 2001
> From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
> Date: Mon, 3 Nov 2025 06:33:01 +0000
> Subject: [PATCH v5 2/4] Introduce PG_DEPRECATED() and deprecate
>  XLogRecPtrIsInvalid()

The uppercase name looks a bit ugly.  We use lowercase for other uses of
__attribute__, e.g. pg_attribute_aligned().  Also, probably add
"attribute" to the name, for consistency with those.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-06T14:27:04Z

Hi,

On Thu, Nov 06, 2025 at 10:06:13AM +0100, Álvaro Herrera wrote:
> On 2025-Nov-06, Bertrand Drouvot wrote:
> 
> > Subject: [PATCH v5 1/4] Introduce XLogRecPtrIsValid() and replace
> >  XLogRecPtrIsInvalid() calls
> 
> > XLogRecPtrIsInvalid() is inconsistent with the affirmative form of other
> > *IsValid() macros and leads to awkward double negative.
> > 
> > This commit introduces XLogRecPtrIsValid() and replace all the
> > XLogRecPtrIsInvalid() calls.
> > 
> > It also adds a comment mentioning that new code should use XLogRecPtrIsValid()
> > instead of XLogRecPtrIsInvalid() and that XLogRecPtrIsInvalid() could be
> > deprecated in the future.
> 
> I think we should do this in two steps.  First, introduce
> XLogRecPtrIsValid(), don't use it anywhere, backpatch this one.  This
> would alleviate potential backpatching pains when using the new macro in
> future bugfixes. 

I see, I would have introduced XLogRecPtrIsInvalid() on the back branches only
if there is a need to (a bugfix that would make use of it). But yeah, I agree
that would add extra "unnecessary" work, so done as you suggested in the
attached. I checked that 0001 apply on the [14-18]_STABLE branches successfully.

> The uppercase name looks a bit ugly.  We use lowercase for other uses of
> __attribute__, e.g. pg_attribute_aligned().  Also, probably add
> "attribute" to the name, for consistency with those.

Right, replaced by pg_attribute_deprecated() in the attached.

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-06T19:48:11Z

On 2025-Nov-06, Bertrand Drouvot wrote:

> I see, I would have introduced XLogRecPtrIsInvalid() on the back branches only
> if there is a need to (a bugfix that would make use of it). But yeah, I agree
> that would add extra "unnecessary" work, so done as you suggested in the
> attached. I checked that 0001 apply on the [14-18]_STABLE branches successfully.

Okay, thanks, I have applied that one to all stable branches, except I
didn't add the judgemental comment about XLogRecPtrIsInvalid().

I also pushed 0002+0004+0005 together as one commit, so now we have
XLogRecPtrIsValid() everywhere.

I did a couple of minor transformations, where the new code would end
doing "!XLogRecPtrIsValid(x) ? A : B" it seems clearer to remove the
negation and invert the other two arguments in the ternary.  We also had
this assertion,

-   Assert(XLogRecPtrIsInvalid(state->istartpoint) == (state->istarttli == 0));

which was being transformed to have a negation.  I chose to negate the
other side of the equality instead, that is,

+   Assert(XLogRecPtrIsValid(state->istartpoint) == (state->istarttli != 0));

which also seems clearer.

Now only 0003 remains ... I would change the complaining version to 21
there, because why not?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-07T06:17:27Z

Hi,

On Thu, Nov 06, 2025 at 08:48:11PM +0100, Álvaro Herrera wrote:
> On 2025-Nov-06, Bertrand Drouvot wrote:
> 
> > I see, I would have introduced XLogRecPtrIsInvalid() on the back branches only
> > if there is a need to (a bugfix that would make use of it). But yeah, I agree
> > that would add extra "unnecessary" work, so done as you suggested in the
> > attached. I checked that 0001 apply on the [14-18]_STABLE branches successfully.
> 
> Okay, thanks, I have applied that one to all stable branches, except I
> didn't add the judgemental comment about XLogRecPtrIsInvalid().
> 
> I also pushed 0002+0004+0005 together as one commit, so now we have
> XLogRecPtrIsValid() everywhere.

Thanks!

> I did a couple of minor transformations, where the new code would end
> doing "!XLogRecPtrIsValid(x) ? A : B" it seems clearer to remove the
> negation and invert the other two arguments in the ternary.  We also had
> this assertion,
> 
> -   Assert(XLogRecPtrIsInvalid(state->istartpoint) == (state->istarttli == 0));
> 
> which was being transformed to have a negation.  I chose to negate the
> other side of the equality instead, that is,
> 
> +   Assert(XLogRecPtrIsValid(state->istartpoint) == (state->istarttli != 0));
> 
> which also seems clearer.

Agree, will modify the .cocci scripts that way.

> Now only 0003 remains ... I would change the complaining version to 21
> there, because why not?

Now that XLogRecPtrIsValid() is available in back branches, I agree that we
can be less conservative and not wait until v24. v21 looks like good timing to
me.

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-07T13:37:32Z

On 2025-Nov-07, Bertrand Drouvot wrote:

> Agree, will modify the .cocci scripts that way.

I just noticed that we missed this ... maybe you want to include it also?

diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index a0c79958fd5..1f11c8646f5 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -355,7 +355,7 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
 	pg_read_barrier();
 	Assert(dlist_node_is_detached(&MyProc->syncRepLinks));
 	MyProc->syncRepState = SYNC_REP_NOT_WAITING;
-	MyProc->waitLSN = 0;
+	MyProc->waitLSN = InvalidXLogRecPtr;
 
 	/* reset ps display to remove the suffix */
 	if (update_process_title)
@@ -1028,7 +1028,7 @@ SyncRepQueueIsOrderedByLSN(int mode)
 
 	Assert(mode >= 0 && mode < NUM_SYNC_REP_WAIT_MODE);
 
-	lastLSN = 0;
+	lastLSN = InvalidXLogRecPtr;
 
 	dlist_foreach(iter, &WalSndCtl->SyncRepQueue[mode])
 	{
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 1504fafe6d8..ce0d6a7539c 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -509,7 +509,7 @@ InitProcess(void)
 	MyProc->recoveryConflictPending = false;
 
 	/* Initialize fields for sync rep */
-	MyProc->waitLSN = 0;
+	MyProc->waitLSN = InvalidXLogRecPtr;
 	MyProc->syncRepState = SYNC_REP_NOT_WAITING;
 	dlist_node_init(&MyProc->syncRepLinks);
 

> Now that XLogRecPtrIsValid() is available in back branches, I agree that we
> can be less conservative and not wait until v24. v21 looks like good timing to
> me.

Cool,  please resubmit.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"They proved that being American is not just for some people"
                                               (George Takei)

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-07T15:03:03Z

Hi,

On Fri, Nov 07, 2025 at 02:37:32PM +0100, Álvaro Herrera wrote:
> On 2025-Nov-07, Bertrand Drouvot wrote:
> 
> > Agree, will modify the .cocci scripts that way.
> 
> I just noticed that we missed this ... maybe you want to include it also?
> 
> -	MyProc->waitLSN = 0;
> +	MyProc->waitLSN = InvalidXLogRecPtr;
>  
> -	lastLSN = 0;
> +	lastLSN = InvalidXLogRecPtr;
>  
> -	MyProc->waitLSN = 0;
> +	MyProc->waitLSN = InvalidXLogRecPtr;

Yeah, that's another story here that is worth to look at too. Will do.

I'm currently working on the RegProcedureIsValid() and OidIsValid() cases,
will share once done.

> 
> > Now that XLogRecPtrIsValid() is available in back branches, I agree that we
> > can be less conservative and not wait until v24. v21 looks like good timing to
> > me.
> 
> Cool,  please resubmit.

Sure, done in the attached.

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Peter Eisentraut <peter@eisentraut.org> — 2025-11-07T16:05:11Z

On 07.11.25 16:03, Bertrand Drouvot wrote:
> +/*
> + * Mark a declaration as deprecated with a custom message. The compiler will
> + * emit a warning when the deprecated entity is used.
> + */
> +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L || \
> +defined(__cplusplus) && __cplusplus >= 201402L

This could use some parentheses to disambiguate the && and ||.

Also the second line could be indented (or just put it on one line).

> +#define pg_attribute_deprecated(msg) [[deprecated(msg)]]
> +#elif defined(__GNUC__) || defined(__clang__)

The __clang__ part is not needed, because clang defines __GNUC__ also.

> +#define pg_attribute_deprecated(msg) __attribute__((deprecated(msg)))
> +#elif defined(_MSC_VER)
> +#define pg_attribute_deprecated(msg) __declspec(deprecated(msg))
> +#else
> +#define pg_attribute_deprecated(msg)
> +#endif

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-07T17:15:09Z

Hi,

On Fri, Nov 07, 2025 at 05:05:11PM +0100, Peter Eisentraut wrote:
> On 07.11.25 16:03, Bertrand Drouvot wrote:
> > +/*
> > + * Mark a declaration as deprecated with a custom message. The compiler will
> > + * emit a warning when the deprecated entity is used.
> > + */
> > +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L || \
> > +defined(__cplusplus) && __cplusplus >= 201402L
> 
> This could use some parentheses to disambiguate the && and ||.
> 
> Also the second line could be indented (or just put it on one line).

Agree that it could be more clear. Done that way in the attached (using only
one line as it looks more readable).

> > +#define pg_attribute_deprecated(msg) [[deprecated(msg)]]
> > +#elif defined(__GNUC__) || defined(__clang__)
> 
> The __clang__ part is not needed, because clang defines __GNUC__ also.

Good catch, thanks! Fixed in the attach.

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2025-11-07T17:18:41Z

Peter Eisentraut <peter@eisentraut.org> writes:

> On 07.11.25 16:03, Bertrand Drouvot wrote:
>
>> +#define pg_attribute_deprecated(msg) [[deprecated(msg)]]
>> +#elif defined(__GNUC__) || defined(__clang__)
>
> The __clang__ part is not needed, because clang defines __GNUC__ also.

Or, to avoid having to know this, how about __has_attribute(deprecated)?

- ilmari

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-07T17:22:59Z

Hi,

On Fri, Nov 07, 2025 at 03:03:03PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Fri, Nov 07, 2025 at 02:37:32PM +0100, Álvaro Herrera wrote:
> > On 2025-Nov-07, Bertrand Drouvot wrote:
> > 
> > > Agree, will modify the .cocci scripts that way.
> > 
> > I just noticed that we missed this ... maybe you want to include it also?
> > 
> > -	MyProc->waitLSN = 0;
> > +	MyProc->waitLSN = InvalidXLogRecPtr;
> >  
> > -	lastLSN = 0;
> > +	lastLSN = InvalidXLogRecPtr;
> >  
> > -	MyProc->waitLSN = 0;
> > +	MyProc->waitLSN = InvalidXLogRecPtr;
> 
> Yeah, that's another story here that is worth to look at too. Will do.

What do you think of the attached? It contains the ones you mentioned and some
others. The patch attached has been generated by the .cocci script [1].

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_literal_0_assignement_with_InvalidXLogRecPtr.cocci

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-10T07:57:39Z

Hi,

On Fri, Nov 07, 2025 at 05:18:41PM +0000, Dagfinn Ilmari Mannsåker wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
> 
> > On 07.11.25 16:03, Bertrand Drouvot wrote:
> >
> >> +#define pg_attribute_deprecated(msg) [[deprecated(msg)]]
> >> +#elif defined(__GNUC__) || defined(__clang__)
> >
> > The __clang__ part is not needed, because clang defines __GNUC__ also.
> 
> Or, to avoid having to know this, how about __has_attribute(deprecated)?
> 

Thanks for looking at it! I did some research and found that some older GCC
versions did support the deprecated attribute (for example GCC 4.5 added support
to the extra message, see [1]) but not __has_attribute (introduced in GCC 5, see
[2]). So for example here, we'd have a 4.5-4.9 gap. 

Then to be on the safe side of things I think it's better to not use
__has_attribute() for the deprecated attribute.

I added a comment in the attached though.

[1]: https://gcc.gnu.org/gcc-4.5/changes.html
[2]: https://gcc.gnu.org/gcc-5/changes.html

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-13T12:55:01Z

On 2025-Nov-07, Bertrand Drouvot wrote:

> What do you think of the attached? It contains the ones you mentioned and some
> others. The patch attached has been generated by the .cocci script [1].
> 
> [1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_literal_0_assignement_with_InvalidXLogRecPtr.cocci

Hmm, I tried to recreate your patch using this .cocci file, and in my
run, there's a few changes in your patch that my spatch run didn't
detect.  I wonder if that's because my spatch version is buggy, or
because you hacked the .cocci file beyond what's in your github repo.

In case you're curious, here's two commits: what I got with my spatch
run as a first step, and then the patch you sent on top of that.

(I'm wondering if I should reproduce your previous patches in case there
were also differences there.  Life is short though.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-13T14:11:08Z

On Fri, Nov 07, 2025 at 02:37:32PM +0100, Álvaro Herrera wrote:

> Hmm, I tried to recreate your patch using this .cocci file, and in my
> run, there's a few changes in your patch that my spatch run didn't
> detect.  I wonder if that's because my spatch version is buggy, or
> because you hacked the .cocci file beyond what's in your github repo.

spatch needs to be run with --recursive-includes (so that the .cocci script
is able to collect information from all the structs of interest). The header
comment in the .cocci script did not mention that: just fixed).

But then I realized that if I run spatch that way:

spatch --sp-file replace_literal_0_assignement_with_InvalidXLogRecPtr.cocci \
      --dir /path/to/postgres/src \
      -I /path/to/postgres/src/include \
      --recursive-includes \
      > replace.patch

Then some headers were not included (no clue as to why). But if I run spatch on
the .c files one by one with the --recursive-includes then it works (i.e all
the headers of interest are included).

So, I created [1] to run spatch one by one on all the .c files (in parallel).

To produce the patch that I shared I ran:

./run_parallel.sh /absolute_path_to/replace_literal_0_assignement_with_InvalidXLogRecPtr.cocci -j 32

(patch can be found in /path/to/postgres once completed).

> (I'm wondering if I should reproduce your previous patches in case there
> were also differences there.  Life is short though.)

I guess/hope you'll get the same results if you use run_parallel.sh as mentioned
above.

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/wrappers/run_parallel.sh

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-17T13:12:24Z

Hi,

On Thu, Nov 13, 2025 at 02:11:08PM +0000, Bertrand Drouvot wrote:
> On Fri, Nov 07, 2025 at 02:37:32PM +0100, Álvaro Herrera wrote:
> 
> I guess/hope you'll get the same results if you use run_parallel.sh as mentioned
> above.

The .cocci script had an issue (not always detecting correctly direct member
access).

The script has been updated and the patch too (finding a new replacement in
gist.c).

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-17T17:39:37Z

Hi,

On Mon, Nov 17, 2025 at 01:12:24PM +0000, Bertrand Drouvot wrote:
> The script has been updated and the patch too (finding a new replacement in
> gist.c).

Finally, adding GistNSN to the game (as a typedef XLogRecPtr) gives the attached.

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-18T09:06:06Z

Hi,

On Fri, Nov 07, 2025 at 03:03:03PM +0000, Bertrand Drouvot wrote:
> I'm currently working on the RegProcedureIsValid() and OidIsValid() cases,
> will share once done.

here they are, I'm not creating a new thread for those as this is the same
kind of ideas (but for other types) but can create a dedicated one if you prefer.

That's a lot of changes so it is split in multiple patches to ease the review:

0001: makes use of RegProcedureIsValid() instead of direct comparisons with
InvalidOid or literal 0. That leads to some implicit boolean conversion to be
replaced by RegProcedureIsValid() checks. There are no literal 0 assignments on
RegProcedure type.

0002: makes use of OidIsValid() instead of direct comparisons with InvalidOid
or literal 0. That leads to some implicit boolean conversion to be replaced by
OidIsValid() checks. It covers the majority of cases.

0003: same as 0002 but for pointers/array.

0004: same as 0002 but for CATALOG (FormData_xxx) structs.

0005: same as 0002 but for functions returning Oid.

0006: same as 0002 but for remaining edge cases that have been done manually.

0007: replace ternary operators with !OidIsValid() rnd negated OidIsValid
equality to use positive logic.

0008: replace literal 0 with InvalidOid for Oid assignments.

Remarks:

- those patches (except 0006) have been generated using the .cocci scripts in
[1] (note that for the CATALOG patch, the macro_file.txt has to be used, see
the script header).

- comments are not changed, so there is still a few that contains "== InvalidOid"
while the underlying code is now using OidIsValid(). We may want to change
the comments too. I don't have a strong opinion on it just a small preference
to change the comments too. Thoughts?

Please note that, once the patch is applied, there is one InvalidOid direct
comparison done that is not on "Oid" or "RegProcedure" types:

$ git grep "!= InvalidOid"
src/backend/storage/lmgr/lock.c:        (locktag)->locktag_field1 != InvalidOid && \

I think this one should be replaced by != 0 instead because:

- locktag_field1 is uint32 (not RegProcedure or Oid)
- it can be assigned non Oid values (like xid, procNumber)

"
src/include/storage/lock.h:     uint32          locktag_field1; /* a 32-bit ID field */
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (xid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (vxid).procNumber, \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (xid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (id1), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
"

While the direct comparison to InvalidOid is technically correct, it is
confusing semantically because the field doesn't always contain an OID value.

I'll now look at the TransactionIdIsValid() cases.

[1]: https://github.com/bdrouvot/coccinelle_on_pg/tree/main/InvalidOid

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Peter Eisentraut <peter@eisentraut.org> — 2025-11-18T15:54:32Z

On 18.11.25 10:06, Bertrand Drouvot wrote:
> Hi,
> 
> On Fri, Nov 07, 2025 at 03:03:03PM +0000, Bertrand Drouvot wrote:
>> I'm currently working on the RegProcedureIsValid() and OidIsValid() cases,
>> will share once done.
> 
> here they are, I'm not creating a new thread for those as this is the same
> kind of ideas (but for other types) but can create a dedicated one if you prefer.

I don't like this change.

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.

For OidIsValid etc., I don't think this improves the notation.  It is 
well understood that InvalidOid is 0.  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.  But we're certainly not going to 
introduce, uh, if (PointerIsValid(foo)), and in fact we just removed 
that!  What you're proposing here seem quite analogous but in the 
opposite direction.

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-18T17:14:02Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-18T17:57:33Z

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)

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-19T07:58:02Z

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Robert Haas <robertmhaas@gmail.com> — 2025-11-19T16:14:53Z

On Thu, Nov 6, 2025 at 2:48 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> Okay, thanks, I have applied that one to all stable branches, except I
> didn't add the judgemental comment about XLogRecPtrIsInvalid().

I'm rather late to the party here, but for what it's worth, I don't
really think this was a good idea. Anyone who wants to write
out-of-core code that works in the back-branches must still write it
the old way, or it will potentially fail on older minor releases. Over
the alternative actually chosen, I would have preferred (a) not doing
this project at all or (b) making a hard switch in master to use the
new macro everywhere and remove the old one, while leaving the
back-branches unchanged or (c) dropping the use of the macro
altogether, in that order of preference.

That sad, I'm not arguing for a revert. My basic position is that this
wasn't worth the switching cost, not that it was intrinsically a bad
idea.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-19T16:49:42Z

On 2025-Nov-19, Robert Haas wrote:

> On Thu, Nov 6, 2025 at 2:48 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > Okay, thanks, I have applied that one to all stable branches, except
> > I didn't add the judgemental comment about XLogRecPtrIsInvalid().
> 
> I'm rather late to the party here, but for what it's worth, I don't
> really think this was a good idea. Anyone who wants to write
> out-of-core code that works in the back-branches must still write it
> the old way, or it will potentially fail on older minor releases.

No, they don't need to.  Thus far, they can still keep their code the
way it is.  The next patch in the series (not yet committed, but I
intend to get it out at some point, unless there are objections) is
going to add an obsolescence warning when their code is compiled with
Postgres 21 -- by which time the minors without the new macro are going
to be two years old.  Nobody needs to compile their code with minor
releases that old.  So they can fix their code to work with Postgres 21
and with all contemporary minors.  They don't need to ensure that their
code compiles with minors older than that.

We could make that Postgres 22, but I don't think that makes any
practical difference.

Maybe you misunderstood what the patch is doing.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: Consistently use the XLogRecPtrIsInvalid() macro

Robert Haas <robertmhaas@gmail.com> — 2025-11-19T17:27:30Z

On Wed, Nov 19, 2025 at 11:49 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > I'm rather late to the party here, but for what it's worth, I don't
> > really think this was a good idea. Anyone who wants to write
> > out-of-core code that works in the back-branches must still write it
> > the old way, or it will potentially fail on older minor releases.
>
> No, they don't need to.  Thus far, they can still keep their code the
> way it is.

True, but if they write any new code, and care about it compiling with
older minor releases, this is a potential pitfall.

>  The next patch in the series (not yet committed, but I
> intend to get it out at some point, unless there are objections) is
> going to add an obsolescence warning when their code is compiled with
> Postgres 21 -- by which time the minors without the new macro are going
> to be two years old.  Nobody needs to compile their code with minor
> releases that old.  So they can fix their code to work with Postgres 21
> and with all contemporary minors.  They don't need to ensure that their
> code compiles with minors older than that.

Well, if nobody needs to do this, then there's no problem, of course.
I doubt that's true, though.

> We could make that Postgres 22, but I don't think that makes any
> practical difference.
>
> Maybe you misunderstood what the patch is doing.

It's possible, but fundamentally I think it's about replacing
XLogRecPtrIsInvalid with XLogRecPtrIsValid, and what I'm saying is I
wouldn't have chosen to do that. I agree that it would have been
better to do it that way originally, but I disagree with paying the
switching cost, especially in the back-branches.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-11-19T17:47:26Z

Hi,

On Wed, Nov 19, 2025 at 12:27:30PM -0500, Robert Haas wrote:
> On Wed, Nov 19, 2025 at 11:49 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > I'm rather late to the party here, but for what it's worth, I don't
> > > really think this was a good idea. Anyone who wants to write
> > > out-of-core code that works in the back-branches must still write it
> > > the old way, or it will potentially fail on older minor releases.
> >
> > No, they don't need to.  Thus far, they can still keep their code the
> > way it is.
> 
> True, but if they write any new code, and care about it compiling with
> older minor releases, this is a potential pitfall.

Why given that 06edbed4786 has been back patched through 13?

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Andres Freund <andres@anarazel.de> — 2025-11-19T17:53:31Z

On 2025-11-19 12:27:30 -0500, Robert Haas wrote:
> It's possible, but fundamentally I think it's about replacing
> XLogRecPtrIsInvalid with XLogRecPtrIsValid, and what I'm saying is I
> wouldn't have chosen to do that. I agree that it would have been
> better to do it that way originally, but I disagree with paying the
> switching cost, especially in the back-branches.

+1

This just seems like a lot of noise for something at most very mildly odd.

Re: Consistently use the XLogRecPtrIsInvalid() macro

Robert Haas <robertmhaas@gmail.com> — 2025-11-19T19:17:37Z

On Wed, Nov 19, 2025 at 12:47 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> > True, but if they write any new code, and care about it compiling with
> > older minor releases, this is a potential pitfall.
>
> Why given that 06edbed4786 has been back patched through 13?

I do not know how to make the phrase "older minor releases" any more
clear. You and Álvaro seem to be under the impression that nobody will
ever try to compile code written after this change from a point
release that we shipped before this change. While I don't think that
will be a common thing to do, I'm not sure where you get the idea that
older minor releases completely cease to be relevant when we release a
new one. That's just not how it works.

I bet if we look in a few years we'll find modules on PGXN that have
#ifdef logic in them to make sure they can work with both
XLogRecPtrIsInvalid and XLogRecPtrIsValid. Probably most won't; a lot
of extensions don't need either macro anyway. But what do you think
that an extension maintainer is going to do if their build breaks at
some point, on master or in the back-branches? Do you think they're
just going to do a hard switch to the new macro? Because that's not
what I will do if this breaks something I have to maintain. I'll
certainly make it work both ways, somehow or other. And I bet everyone
else will do the same.

And that would be totally fine and reasonable if this were fixing an
actual problem.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Consistently use the XLogRecPtrIsInvalid() macro

Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-19T22:03:59Z

On 2025-Nov-19, Robert Haas wrote:

> I do not know how to make the phrase "older minor releases" any more
> clear.

It's perfectly clear.  I just don't believe this claim.

> You and Álvaro seem to be under the impression that nobody will
> ever try to compile code written after this change from a point
> release that we shipped before this change. While I don't think that
> will be a common thing to do, I'm not sure where you get the idea that
> older minor releases completely cease to be relevant when we release a
> new one. That's just not how it works.

I'm sure compiled versions continue to be relevant, but I highly doubt
anybody compiles afresh with old minors.

> I bet if we look in a few years we'll find modules on PGXN that have
> #ifdef logic in them to make sure they can work with both
> XLogRecPtrIsInvalid and XLogRecPtrIsValid.

Ok, let's wait a few years and see.  My bet is you won't find any.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-01T07:14:42Z

Hi,

On Tue, Nov 18, 2025 at 04:54:32PM +0100, Peter Eisentraut wrote:
> 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.

Agree. Out of curiosity, I searched for pointers and literal zero comparisons
or assignments (with [1]) and found 6 of them.

While literal zero is technically correct, NULL is the semantically appropriate
choice for pointers.

PFA a patch to fix those 6.

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/pointers_and_literal_zero.cocci

Regards,

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

Re: Consistently use the XLogRecPtrIsInvalid() macro

Peter Eisentraut <peter@eisentraut.org> — 2025-12-02T07:44:28Z

On 01.12.25 08:14, Bertrand Drouvot wrote:
> Hi,
> 
> On Tue, Nov 18, 2025 at 04:54:32PM +0100, Peter Eisentraut wrote:
>> 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.
> 
> Agree. Out of curiosity, I searched for pointers and literal zero comparisons
> or assignments (with [1]) and found 6 of them.
> 
> While literal zero is technically correct, NULL is the semantically appropriate
> choice for pointers.
> 
> PFA a patch to fix those 6.

committed (required pgindent)

Re: Consistently use the XLogRecPtrIsInvalid() macro

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-02T14:35:48Z

Hi,

On Tue, Dec 02, 2025 at 08:44:28AM +0100, Peter Eisentraut wrote:
> On 01.12.25 08:14, Bertrand Drouvot wrote:
> > Hi,
> > 
> > On Tue, Nov 18, 2025 at 04:54:32PM +0100, Peter Eisentraut wrote:
> > > 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.
> > 
> > Agree. Out of curiosity, I searched for pointers and literal zero comparisons
> > or assignments (with [1]) and found 6 of them.
> > 
> > While literal zero is technically correct, NULL is the semantically appropriate
> > choice for pointers.
> > 
> > PFA a patch to fix those 6.
> 
> committed (required pgindent)

Thanks! 

Doh, I always run pgindent unless when I think the changes could not break the
indentation.

Note to self: always run pgindent, no thinking required ;-)

Regards,

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