Consistently use the XLogRecPtrIsInvalid() macro
Hi hackers,
While working on refactoring some code in [1]/messages/by-id/CAD21AoB_C6V1PLNs=SuOejgGh1o6ZHJMstD7X4X1b_z==LdH1Q@mail.gmail.com, 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]/messages/by-id/CAH2-WzmxPQAF_ZhwrUo3rzVk3yYj_4mqbgiQXAGGO5nFYV3D8Q@mail.gmail.com).
Thoughts?
[1]: /messages/by-id/CAD21AoB_C6V1PLNs=SuOejgGh1o6ZHJMstD7X4X1b_z==LdH1Q@mail.gmail.com
[2]: /messages/by-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
Attachments:
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.
Show quoted text
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]: /messages/by-id/CAD21AoB_C6V1PLNs=SuOejgGh1o6ZHJMstD7X4X1b_z==LdH1Q@mail.gmail.com
[2]: /messages/by-id/CAH2-WzmxPQAF_ZhwrUo3rzVk3yYj_4mqbgiQXAGGO5nFYV3D8Q@mail.gmail.comRegards,
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]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_XLogRecPtrIsInvalid.cocci.
- 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]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_InvalidXLogRecPtr_comparisons.cocci.
- 0004 replaces literal 0 comparisons on XLogRecPtr with XLogRecPtrIsValid(). The
replacement changes have been generated by the Coccinelle script [3]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_literal_0_comparisons.cocci.
[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
Attachments:
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]/messages/by-id/4992828D.8000307@gmx.net (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]: /messages/by-id/4992828D.8000307@gmx.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
Attachments:
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
Attachments:
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
Attachments:
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/
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() callsXLogRecPtrIsInvalid() 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
Attachments:
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)
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
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)
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
Attachments:
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.
Show quoted text
+#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
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 >= 201402LThis 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
Attachments:
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
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]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
Attachments:
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]https://gcc.gnu.org/gcc-4.5/changes.html) but not __has_attribute (introduced in GCC 5, see
[2]: https://gcc.gnu.org/gcc-5/changes.html
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
Attachments:
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].
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/
Attachments:
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]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/wrappers/run_parallel.sh 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
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
Attachments:
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
Attachments:
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]: https://github.com/bdrouvot/coccinelle_on_pg/tree/main/InvalidOid
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
Attachments:
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.
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
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)
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]/messages/by-id/CACJufxGmsphWX150CxMU6KSed-x2fmTH3UpCwHpedGmjV1Biug@mail.gmail.com 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]: /messages/by-id/CACJufxGmsphWX150CxMU6KSed-x2fmTH3UpCwHpedGmjV1Biug@mail.gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
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
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/
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
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
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.
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
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/
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]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/pointers_and_literal_zero.cocci) 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
Attachments:
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)
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