Обсуждение: Consistently use the XLogRecPtrIsInvalid() macro
Hi hackers, While working on refactoring some code in [1], one of the changes was: - if (initial_restart_lsn != InvalidXLogRecPtr && - initial_restart_lsn < oldestLSN) + XLogRecPtr restart_lsn = s->data.restart_lsn; + + if (restart_lsn != InvalidXLogRecPtr && + restart_lsn < oldestLSN) Sawada-san suggested to use the XLogRecPtrIsInvalid() macro here. But this != InvalidXLogRecPtr check was existing code, so why not consistently use XLogRecPtrIsInvalid() where we check equality against InvalidXLogRecPtr? At the time the current XLogRecPtrIsInvalid() has been introduced (0ab9d1c4b316) all the InvalidXLogRecPtr equality checks were done using XLogRecPtrIsInvalid(). But since, it has changed: I looked at it and this is not the case anymore in 20 files. PFA, patches to $SUBJECT. To ease the review, I created one patch per modified file. I suspect the same approach could be applied to some other macros too. Let's start with XLogRecPtrIsInvalid() first. I think that's one of the things we could do once a year, like Peter does with his annual "clang-tidy" check ([2]). Thoughts? [1]: https://www.postgresql.org/message-id/CAD21AoB_C6V1PLNs%3DSuOejgGh1o6ZHJMstD7X4X1b_z%3D%3DLdH1Q%40mail.gmail.com [2]: https://www.postgresql.org/message-id/CAH2-WzmxPQAF_ZhwrUo3rzVk3yYj_4mqbgiQXAGGO5nFYV3D8Q@mail.gmail.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
- v1-0001-make-use-of-XLogRecPtrIsInvalid-in-rewriteheap.c.patch
- v1-0002-make-use-of-XLogRecPtrIsInvalid-in-twophase.c.patch
- v1-0003-make-use-of-XLogRecPtrIsInvalid-in-xlog.c.patch
- v1-0004-make-use-of-XLogRecPtrIsInvalid-in-xloginsert.c.patch
- v1-0005-make-use-of-XLogRecPtrIsInvalid-in-xlogreader.c.patch
- v1-0006-make-use-of-XLogRecPtrIsInvalid-in-xlogrecovery.c.patch
- v1-0007-make-use-of-XLogRecPtrIsInvalid-in-xlogutils.c.patch
- v1-0008-make-use-of-XLogRecPtrIsInvalid-in-pg_subscriptio.patch
- v1-0009-make-use-of-XLogRecPtrIsInvalid-in-logical.c.patch
- v1-0010-make-use-of-XLogRecPtrIsInvalid-in-logicalfuncs.c.patch
- v1-0011-make-use-of-XLogRecPtrIsInvalid-in-origin.c.patch
- v1-0012-make-use-of-XLogRecPtrIsInvalid-in-proto.c.patch
- v1-0013-make-use-of-XLogRecPtrIsInvalid-in-reorderbuffer..patch
- v1-0014-make-use-of-XLogRecPtrIsInvalid-in-snapbuild.c.patch
- v1-0015-make-use-of-XLogRecPtrIsInvalid-in-slot.c.patch
- v1-0016-make-use-of-XLogRecPtrIsInvalid-in-slotfuncs.c.patch
- v1-0017-make-use-of-XLogRecPtrIsInvalid-in-walsender.c.patch
- v1-0018-make-use-of-XLogRecPtrIsInvalid-in-pg_receivewal..patch
- v1-0019-make-use-of-XLogRecPtrIsInvalid-in-pg_recvlogical.patch
- v1-0020-make-use-of-XLogRecPtrIsInvalid-in-pg_waldump.c.patch
On 10/28/25 4:13 PM, Bertrand Drouvot wrote: > Hi hackers, > > While working on refactoring some code in [1], one of the changes was: > > - if (initial_restart_lsn != InvalidXLogRecPtr && > - initial_restart_lsn < oldestLSN) > + XLogRecPtr restart_lsn = s->data.restart_lsn; > + > + if (restart_lsn != InvalidXLogRecPtr && > + restart_lsn < oldestLSN) > > Sawada-san suggested to use the XLogRecPtrIsInvalid() macro here. > > But this != InvalidXLogRecPtr check was existing code, so why not consistently > use XLogRecPtrIsInvalid() where we check equality against InvalidXLogRecPtr? > > At the time the current XLogRecPtrIsInvalid() has been introduced (0ab9d1c4b316) > all the InvalidXLogRecPtr equality checks were done using XLogRecPtrIsInvalid(). > > But since, it has changed: I looked at it and this is not the case anymore in > 20 files. > > PFA, patches to $SUBJECT. To ease the review, I created one patch per modified > file. > > I suspect the same approach could be applied to some other macros too. Let's > start with XLogRecPtrIsInvalid() first. > Agree. This patch has made the code style more consistent. And using such macros is also in line with the usual practice. Just like OidIsValid and TransactionIdIsValid. > I think that's one of the things we could do once a year, like Peter does with > his annual "clang-tidy" check ([2]). > > Thoughts? > > [1]: https://www.postgresql.org/message-id/CAD21AoB_C6V1PLNs%3DSuOejgGh1o6ZHJMstD7X4X1b_z%3D%3DLdH1Q%40mail.gmail.com > [2]: https://www.postgresql.org/message-id/CAH2-WzmxPQAF_ZhwrUo3rzVk3yYj_4mqbgiQXAGGO5nFYV3D8Q@mail.gmail.com > > Regards, >
On 28/10/2025 10:53, Quan Zongliang wrote: > On 10/28/25 4:13 PM, Bertrand Drouvot wrote: >> While working on refactoring some code in [1], one of the changes was: >> >> - if (initial_restart_lsn != InvalidXLogRecPtr && >> - initial_restart_lsn < oldestLSN) >> + XLogRecPtr restart_lsn = s->data.restart_lsn; >> + >> + if (restart_lsn != InvalidXLogRecPtr && >> + restart_lsn < oldestLSN) >> >> Sawada-san suggested to use the XLogRecPtrIsInvalid() macro here. >> >> But this != InvalidXLogRecPtr check was existing code, so why not >> consistently use XLogRecPtrIsInvalid() where we check equality >> against InvalidXLogRecPtr? >> >> At the time the current XLogRecPtrIsInvalid() has been introduced >> (0ab9d1c4b316) all the InvalidXLogRecPtr equality checks were done >> using XLogRecPtrIsInvalid(). >> >> But since, it has changed: I looked at it and this is not the case >> anymore in 20 files. >> >> PFA, patches to $SUBJECT. To ease the review, I created one patch >> per modified file. >> >> I suspect the same approach could be applied to some other macros >> too. Let's start with XLogRecPtrIsInvalid() first. > > Agree. This patch has made the code style more consistent. And using > such macros is also in line with the usual practice. Just like > OidIsValid and TransactionIdIsValid. Back in the day, XLogRecPtrIsInvalid() was needed because XLogRecPtr was a struct. 'x == InvalidXLogRecPtr' simply did not work. I don't see much need for it nowadays. In fact I wonder if we should go in the other direction and replace XLogRecPtrIsInvalid(x) calls with 'x == InvalidXLogRecPtr'. It's also a bit cumbersome that we have XLogRecPtrIsInvalid() rather than XLogRecPtrIsValid(). That's inconsistent with OidIsValid and TransactionIdInValid, and it leads to an awkward double negative 'if (!XLogRecPtrIsInvalid(x))' if you want to check that 'x' is valid. Overall I'm inclined to do nothing. But if anything, perhaps introduce XLogRecPtrIsValid(x) and switch to that, or replace XLogRecPtrIsInvalid(x) calls with 'x == InvalidXLogRecPtr' - Heikki
Hi, On Tue, Oct 28, 2025 at 01:40:24PM +0200, Heikki Linnakangas wrote: > On 28/10/2025 10:53, Quan Zongliang wrote: > > On 10/28/25 4:13 PM, Bertrand Drouvot wrote: > > > While working on refactoring some code in [1], one of the changes was: > > > > > > - if (initial_restart_lsn != InvalidXLogRecPtr && > > > - initial_restart_lsn < oldestLSN) > > > + XLogRecPtr restart_lsn = s->data.restart_lsn; > > > + > > > + if (restart_lsn != InvalidXLogRecPtr && > > > + restart_lsn < oldestLSN) > > > > > > Sawada-san suggested to use the XLogRecPtrIsInvalid() macro here. > > > > > > But this != InvalidXLogRecPtr check was existing code, so why not > > > consistently use XLogRecPtrIsInvalid() where we check equality > > > against InvalidXLogRecPtr? > > > > > > At the time the current XLogRecPtrIsInvalid() has been introduced > > > (0ab9d1c4b316) all the InvalidXLogRecPtr equality checks were done > > > using XLogRecPtrIsInvalid(). > > > > > > But since, it has changed: I looked at it and this is not the case > > > anymore in 20 files. > > > > > > PFA, patches to $SUBJECT. To ease the review, I created one patch > > > per modified file. > > > > > > I suspect the same approach could be applied to some other macros > > > too. Let's start with XLogRecPtrIsInvalid() first. > > > > Agree. This patch has made the code style more consistent. And using > > such macros is also in line with the usual practice. Just like > > OidIsValid and TransactionIdIsValid. > > Back in the day, XLogRecPtrIsInvalid() was needed because XLogRecPtr was a > struct. 'x == InvalidXLogRecPtr' simply did not work. Thank you both for your feedback! > I don't see much need > for it nowadays. In fact I wonder if we should go in the other direction and > replace XLogRecPtrIsInvalid(x) calls with 'x == InvalidXLogRecPtr'. That would be an option to ensure code consistency (related tp XLogRecPtr checks) that will be hard to break (unless someone re-introduces a similar macro). But OTOH that would introduce some kind of inconsistency with OidIsValid() and TransactionIdIsValid() for example. > It's also a bit cumbersome that we have XLogRecPtrIsInvalid() rather than > XLogRecPtrIsValid(). That's inconsistent with OidIsValid and > TransactionIdInValid, and it leads to an awkward double negative 'if > (!XLogRecPtrIsInvalid(x))' if you want to check that 'x' is valid. Agree. > Overall I'm inclined to do nothing. But if anything, perhaps introduce > XLogRecPtrIsValid(x) and switch to that, or replace XLogRecPtrIsInvalid(x) > calls with 'x == InvalidXLogRecPtr' I do prefer to introduce XLogRecPtrIsValid(x) and switch to that. Then, do the same kind of work on OidIsValid() and TransactionIdIsValid() and add an annual check. Idea is to get some code consistency while keeping macros which are valuable for readability and centralize changes if any need to be done in the way we check their validity. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Tue, Oct 28, 2025 at 01:40:24PM +0200, Heikki Linnakangas wrote: > It's also a bit cumbersome that we have XLogRecPtrIsInvalid() rather than > XLogRecPtrIsValid(). That's inconsistent with OidIsValid and > TransactionIdInValid, and it leads to an awkward double negative 'if > (!XLogRecPtrIsInvalid(x))' if you want to check that 'x' is valid. > > Overall I'm inclined to do nothing. But if anything, perhaps introduce > XLogRecPtrIsValid(x) and switch to that, or replace XLogRecPtrIsInvalid(x) > calls with 'x == InvalidXLogRecPtr' The annoying part with eliminating XLogRecPtrIsInvalid() or replacing it is that a bunch of external code would be broken, particularly backup tools. I'd rather leave the beast alone. -- Michael
Вложения
On 2025-Oct-28, Michael Paquier wrote: > The annoying part with eliminating XLogRecPtrIsInvalid() or replacing > it is that a bunch of external code would be broken, particularly > backup tools. I'd rather leave the beast alone. Well, we don't have to remove it right away. We can simply not use it. And maybe if the compiler supports it, make a static inline function in the header with the [[deprecated]] attribute or __attribute__((deprecated)) so that the tool developers get a warning and migrate to using the new one. Then in a few years we remove it. BTW we could use Coccinelle to replace all the XLogRecPtrIsInvalid() calls with !XLogRecPtrIsValid(), as well as all places comparing an LSN to InvalidXLogRecPtr or literal zero. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Hi, On Tue, Oct 28, 2025 at 04:05:34PM +0200, Álvaro Herrera wrote: > > BTW we could use Coccinelle to replace all the XLogRecPtrIsInvalid() > calls with !XLogRecPtrIsValid(), as well as all places comparing an LSN > to InvalidXLogRecPtr or literal zero. I did v1 the old way (shell script) and did not think about using Coccinelle for this. That's a good idea and well suited for this purpose: I'll work on it. Thanks for the suggestion! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 28.10.25 13:33, Bertrand Drouvot wrote: > I do prefer to introduce XLogRecPtrIsValid(x) and switch to that. Then, do the > same kind of work on OidIsValid() and TransactionIdIsValid() and add an annual > check. > > Idea is to get some code consistency while keeping macros which are valuable for > readability and centralize changes if any need to be done in the way we check > their validity. If we wanted real type safety, we could turn XLogRecPtr back into a struct, and then enforce the use of XLogRecPtrIsValid() and similar. Otherwise, we should just acknowledge that it's an integer and use integer code to deal with it. These *IsValid() and similar macros that are there for "readability" but are not actually enforced other than by some developers' willpower are just causing more work and inconsistency in the long run.
Hi, On Tue, Oct 28, 2025 at 05:57:54PM +0000, Bertrand Drouvot wrote: > Hi, > > On Tue, Oct 28, 2025 at 04:05:34PM +0200, Álvaro Herrera wrote: > > > > BTW we could use Coccinelle to replace all the XLogRecPtrIsInvalid() > > calls with !XLogRecPtrIsValid(), as well as all places comparing an LSN > > to InvalidXLogRecPtr or literal zero. > > I did v1 the old way (shell script) and did not think about using > Coccinelle for this. That's a good idea and well suited for this purpose: I'll > work on it. Thanks for the suggestion! PFA a series of patches to implement what has been discussed above i.e: - Introduce XLogRecPtrIsValid() and replace XLogRecPtrIsInvalid() calls: this is done in 0001. The replacement changes have been generated by the Coccinelle script [1]. - 0002 deprecates XLogRecPtrIsInvalid(): it emits a warning message at compilation time if XLogRecPtrIsInvalid() is in use in the code base. - 0003 replaces InvalidXLogRecPtr comparisons with XLogRecPtrIsValid(). The replacement changes have been generated by the Coccinelle script [2]. - 0004 replaces literal 0 comparisons on XLogRecPtr with XLogRecPtrIsValid(). The replacement changes have been generated by the Coccinelle script [3]. [1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_XLogRecPtrIsInvalid.cocci [2]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_InvalidXLogRecPtr_comparisons.cocci [3]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/replace_literal_0_comparisons.cocci Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
Hi, On Wed, Oct 29, 2025 at 05:50:13PM +0100, Peter Eisentraut wrote: > On 28.10.25 13:33, Bertrand Drouvot wrote: > > I do prefer to introduce XLogRecPtrIsValid(x) and switch to that. Then, do the > > same kind of work on OidIsValid() and TransactionIdIsValid() and add an annual > > check. > > > > Idea is to get some code consistency while keeping macros which are valuable for > > readability and centralize changes if any need to be done in the way we check > > their validity. > > If we wanted real type safety, we could turn XLogRecPtr back into a struct, > and then enforce the use of XLogRecPtrIsValid() and similar. Right. That said I think that we'd need an opaque struct to avoid developers doing things like: lsn.value == InvalidXLogRecPtr. If not, we'd still need regular checks to ensure the macro is used. Opaque struct would probably add extra costs too. > Otherwise, we > should just acknowledge that it's an integer and use integer code to deal > with it. These *IsValid() and similar macros that are there for > "readability" but are not actually enforced other than by some developers' > willpower are just causing more work and inconsistency in the long run. That's a good point. Scripts (like the ones shared in [1]) can catch violations, but it's still "manual" enforcement. We don't currently enforce the other *IsValid() macros. I think it would be worth setting up checks for all of them, but I agree that's new ongoing maintenance work. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 30.10.25 10:17, Bertrand Drouvot wrote: > - 0002 deprecates XLogRecPtrIsInvalid(): it emits a warning message at compilation > time if XLogRecPtrIsInvalid() is in use in the code base. Surely this could be factored out in macro in such a way that the warning message is a macro argument and we could reuse this attribute elsewhere in the code. That said, I'm suspicious about marking things deprecated before the replacement is widely available. If an extension has been using XLogRecPtrIsInvalid() until now (which has been best practice), when that extension adds PG19 support, they will either have to backport XLogRecPtrIsValid or turn off deprecation warnings. At least there should be some guidance about what you expect third-party code to do about this.
Hi, On Thu, Oct 30, 2025 at 02:55:51PM +0100, Peter Eisentraut wrote: > On 30.10.25 10:17, Bertrand Drouvot wrote: > > - 0002 deprecates XLogRecPtrIsInvalid(): it emits a warning message at compilation > > time if XLogRecPtrIsInvalid() is in use in the code base. > > Surely this could be factored out in macro in such a way that the warning > message is a macro argument and we could reuse this attribute elsewhere in > the code. Yeah, I thought about it and initially considered waiting until we have another use case. But you're right that it's better to do it from the start. > That said, I'm suspicious about marking things deprecated before the > replacement is widely available. If an extension has been using > XLogRecPtrIsInvalid() until now (which has been best practice), when that > extension adds PG19 support, they will either have to backport > XLogRecPtrIsValid or turn off deprecation warnings. That's a good point, thanks! I agree with your concern. After giving it more thought, I'm inclined to postpone the compiler warning until XLogRecPtrIsValid() has been available for some time. The question is for how long? I did some research, reading [1] (but that's not really identical) or looking at what we did for example for "SPI_prepare_cursor", "SPI_prepare_params" and friends (but again that's not really identical): 844fe9f159a mentioned that they "can perhaps go away at some point" and are documented as deprecated since PG14. So, back at our case, a conservative approach would be to wait for 5 years until the new macro is available on all the supported major versions. That would mean introduce the deprecated attribute in PG24. I don't see waiting for PG24 as an issue: the main goal of the new macro is to be consistent with other *IsValid() ones and avoid "double negation" and that would be achieved in the core code base. For PG19, we could: Add a comment in the code documenting that XLogRecPtrIsInvalid() is deprecated and that we will enforce a "deprecated" attribute on it in PG24. > At least there should > be some guidance about what you expect third-party code to do about this. The alternative of adding the warning immediately with migration guidance would still create "friction". Especially if we deprecate multiple APIs following this new pattern (i.e adding a "deprecated" attribute). As you said, they could backport the macro themselves but that seems like unnecessary friction when we can simply wait. What do you think? [1]: https://www.postgresql.org/message-id/4992828D.8000307%40gmx.net Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On 31.10.25 05:31, Bertrand Drouvot wrote: > For PG19, we could: > > Add a comment in the code documenting that XLogRecPtrIsInvalid() is deprecated > and that we will enforce a "deprecated" attribute on it in PG24. Just a code comment for now seems reasonable. I wouldn't make any predictions about the future in code comments, though.
Hi, On Fri, Oct 31, 2025 at 08:41:46AM +0100, Peter Eisentraut wrote: > On 31.10.25 05:31, Bertrand Drouvot wrote: > > For PG19, we could: > > > > Add a comment in the code documenting that XLogRecPtrIsInvalid() is deprecated > > and that we will enforce a "deprecated" attribute on it in PG24. > > Just a code comment for now seems reasonable. I wouldn't make any > predictions about the future in code comments, though. Done that way in 0001 attached. Also in passing I realized that v2 did miss doing a replacement: "PageGetLSN(page) == InvalidXLogRecPtr" in vacuumlazy.c. I changed it "manually" for now in 0002 and checked that no other replacements is missing. Will fix the associated coccinelle script. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On 2025-Oct-31, Bertrand Drouvot wrote: > After giving it more thought, I'm inclined to postpone the compiler warning > until XLogRecPtrIsValid() has been available for some time. The question is for > how long? Maybe we can mark it so that it becomes obsolete in a future version, #if PG_VERSION_NUM >= 210000 [[obsolete]] #endif XLogRecPtrIsInvalid( .. ) so that people using it today won't get any noise, but once they do get the warning, the versions without the other macro are already out of support, so they can switch to the new one easily. (This presupposes that we'd add the new macro to older branches as well, which shouldn't be a problem.) Only extensions wishing to support PG versions older than we support would have to work slightly harder, but that should be OK. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "I dream about dreams about dreams", sang the nightingale under the pale moon (Sandman)
Hi, On Fri, Oct 31, 2025 at 01:19:50PM +0100, Álvaro Herrera wrote: > On 2025-Oct-31, Bertrand Drouvot wrote: > > > After giving it more thought, I'm inclined to postpone the compiler warning > > until XLogRecPtrIsValid() has been available for some time. The question is for > > how long? > > Maybe we can mark it so that it becomes obsolete in a future version, > > #if PG_VERSION_NUM >= 210000 > [[obsolete]] > #endif > XLogRecPtrIsInvalid( .. ) > > so that people using it today won't get any noise, but once they do get > the warning, the versions without the other macro are already out of > support, so they can switch to the new one easily. (This presupposes > that we'd add the new macro to older branches as well, which shouldn't > be a problem.) Only extensions wishing to support PG versions older > than we support would have to work slightly harder, but that should be OK. Yeah, I did not think of checking PG_VERSION_NUM for a "future" version, that's a good idea! I did it that way in the attached (in 0002) and introduced PG_DEPRECATED() (as suggested by Peter upthread). The version check is done on 24 to ensure that the new macro is available on all the supported major versions. The PG_DEPRECATED() name and location (c.h) look fine to me but maybe there is better suggestions. I think the way it is done in the attached makes sense, it: - introduces PG_DEPRECATED() - provides a use case on how to use it (i.e using a version that is currently in the future) - ensures that XLogRecPtrIsInvalid() deprecation is "enforced" as of version 24 - ensures that not using XLogRecPtrIsInvalid() is documented (so that it should not be used anymore, at least in core, even waiting for version 24) Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
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
Вложения
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() 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
Вложения
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
Вложения
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
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
Вложения
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]. [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
Вложения
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
Вложения
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/
Вложения
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
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
Вложения
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
Вложения
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
Вложения
- v1-0001-Use-RegProcedureIsValid-in-various-places.patch
- v1-0002-Use-OidIsValid-in-various-places.patch
- v1-0003-Use-OidIsValid-in-various-places-for-pointers.patch
- v1-0004-Use-OidIsValid-in-various-places-for-CATALOG.patch
- v1-0005-Use-OidIsValid-in-various-places-for-functions-re.patch
- v1-0006-Use-OidIsValid-in-various-places-for-edge-cases.patch
- v1-0007-Replace-ternary-operators-with-OidIsValid.patch
- v1-0008-Replace-literal-0-with-InvalidOid-for-Oid-assignm.patch
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] 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
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/