Обсуждение: Mark function arguments of type "T *" as "const T *" where possible

Hi hackers,

Several functions in the codebase accept "T *" parameters but do not modify
the pointed-to data.  These have been updated to take "const T *" instead,
improving type safety, making the interfaces clearer about their intent and may
enable additional optimizations.

This change helps the compiler catch accidental modifications and better documents
immutability of arguments.

This patch is the same idea as 8a27d418f8f but for any type of pointers.

Regarding the concern [1] raised during review of 8a27d418f8f about back-patching
pain: I believe the benefits of improved type safety and clearer interfaces
outweigh the inconvenience of merge conflicts, which are typically straightforward
to resolve for signature changes. That said, that's a large patch and I understand
the concern.

To avoid any risks:

- cases that produce -Wdiscarded-qualifiers warnings have been discarded as
they would need more investigation.

- double pointers are excluded to keep the changes straightforward.

- cases that produce new -Wcast-qual warnings have been discarded.

As in 8a27d418f8f, no functional behavior is changed.

FWIW, the patch has been generated with the help of a coccinelle script [2].

Thoughts?

[1]: https://www.postgresql.org/message-id/aNajnjxr7RbpSaMP%40nathan
[2]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/mark_const_where_possible.cocci

Regards,

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

Hi,

On Tue, Dec 09, 2025 at 04:20:57PM +0000, Bertrand Drouvot wrote:
> To avoid any risks:
> 
> - cases that produce -Wdiscarded-qualifiers warnings have been discarded as
> they would need more investigation.
> 
> - double pointers are excluded to keep the changes straightforward.
> 
> - cases that produce new -Wcast-qual warnings have been discarded.

Despite the above precautions, I just realized there are still "unwanted" const
additions (more on that later). I'll fix those and submit a new patch version.

Regards,

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

Hi,

On Wed, Dec 10, 2025 at 06:39:40AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Tue, Dec 09, 2025 at 04:20:57PM +0000, Bertrand Drouvot wrote:
> > To avoid any risks:
> > 
> > - cases that produce -Wdiscarded-qualifiers warnings have been discarded as
> > they would need more investigation.
> > 
> > - double pointers are excluded to keep the changes straightforward.
> > 
> > - cases that produce new -Wcast-qual warnings have been discarded.
> 
> Despite the above precautions, I just realized there are still "unwanted" const
> additions (more on that later).

What I disliked in v1 is that it added const to produce things like:

"
static void
SetAt(const metastring *s, int pos, char c)
{
    if ((pos < 0) || (pos >= s->length))
        return;

    *(s->str + pos) = c;
}
"

While this is technically correct so the compiler does not complain (because 
s->str is a non const pointer and the added const does not apply to
what s->str points to), adding const here defeats the purpose and is misleading.
The functions clearly modify data accessible through the parameter.

In v2, I removed all the ones that are similar to those. I checked and
the only ones are those introduced by v1.

That's still a large patch though (while focusing only on static functions).

Out of curiosity, I looked at the largest patches (based on the number of files changed)
since the last 10 years. If we remove the "Update copyright", Translation updates
and pgindent ones, that gives:

 425 dbbca2cf299 Remove unused #include's from backend .c files
 346 3c49c6facb2 Convert documentation to DocBook XML
 343 578b229718e Remove WITH OIDS support, change oid catalog column visibility.
 337 c29c578908d Don't use SGML empty tags
 333 1b105f9472b Use palloc_object() and palloc_array() in backend code
 278 c5385929593 Make all Perl warnings fatal
 265 e6927270cd1 meson: Add initial version of meson based build system
 234 1ff01b3902c Convert SGML IDs to lower case
 216 2eb4a831e5f Change TRUE/FALSE to true/false
 212 611806cd726 Add trailing commas to enum definitions

It means it would be the 8th largest with 251 files touched in v2. Note that
1b105f9472b is from today.

Thoughts?

Regards,

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

On Wed, Dec 10, 2025 at 9:44 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> Thoughts?

Kneejerk reaction (as someone who wants better const-correctness!): I
suspect that this patch is not practically reviewable for most people.
Especially knowing that the patchset was formed via subtraction of
known-bad cases rather than addition of known-good cases. `const`
needs to be added with intent.

IMO this is especially problematic with our "context bag" structs. As
one example:

>  static int
> -InitializeLDAPConnection(Port *port, LDAP **ldap)
> +InitializeLDAPConnection(const Port *port, LDAP **ldap)

I don't see a good reason to constrain future developers in this way.
Why shouldn't the code that makes LDAP connections be allowed to take
notes inside the Port at some point in the future?

--Jacob

Hi,

On Wed, Dec 10, 2025 at 12:22:22PM -0800, Jacob Champion wrote:
> On Wed, Dec 10, 2025 at 9:44 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > Thoughts?
> 
> Kneejerk reaction (as someone who wants better const-correctness!):

Thanks for sharing your thoughts!

> I suspect that this patch is not practically reviewable for most people.
> Especially knowing that the patchset was formed via subtraction of
> known-bad cases rather than addition of known-good cases. `const`
> needs to be added with intent.

That's fair feedback. The automated approach was meant to catch all of them
but you're right that it sacrifices intentionality.
Also, with the patch in place that would mean "think twice before changing
from const to no const" and that could create doubts and waste of time for future
patch authors.

> IMO this is especially problematic with our "context bag" structs. As
> one example:
> 
> >  static int
> > -InitializeLDAPConnection(Port *port, LDAP **ldap)
> > +InitializeLDAPConnection(const Port *port, LDAP **ldap)
> 
> I don't see a good reason to constrain future developers in this way.
> Why shouldn't the code that makes LDAP connections be allowed to take
> notes inside the Port at some point in the future?

Yeah, that's a good point. Let me try to focus on functions that really 
deserve the change. I could look at function names that contain "copy" or "cmp",
functions that are used for formatting/printing and size/length calculations as
examples.
Any other ideas?

Regards,

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

On Wed, Dec 10, 2025 at 11:22 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> Also, with the patch in place that would mean "think twice before changing
> from const to no const" and that could create doubts and waste of time for future
> patch authors.

Yeah, exactly.

> Let me try to focus on functions that really
> deserve the change. I could look at function names that contain "copy" or "cmp",
> functions that are used for formatting/printing and size/length calculations as
> examples.

Sure, that sounds reasonable, and I would hope that those sorts of
functions are not very high on the list of backport contention.

> Any other ideas?

Just that the most useful const additions (IMHO) are going to be
places at the top of a big hierarchy, where callers expect something
not to be modified, but the lowest levels are too far removed from
that expectation for developers to easily remember. I imagine those
cases are not going to be easy to find automatically (but that
shouldn't discourage incremental improvements that can be found that
way).

Thanks!
--Jacob

Hi,

On Thu, Dec 11, 2025 at 09:51:49AM -0800, Jacob Champion wrote:
> On Wed, Dec 10, 2025 at 11:22 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > Let me try to focus on functions that really
> > deserve the change. I could look at function names that contain "copy" or "cmp",
> > functions that are used for formatting/printing and size/length calculations as
> > examples.
> 
> Sure, that sounds reasonable, and I would hope that those sorts of
> functions are not very high on the list of backport contention.

So, with a filter like:

"^.*(cmp|copy|Cmp|Copy|Control|control|Check|check|Size|size|Length|length).*$";

(maybe other filters will come to mind)

that gives:

 36 files changed, 92 insertions(+), 78 deletions(-)

I did not check the details, just the repartition and that gives:

cmp: 3 functions
copy: 18
control: 1
check: 35
size: 15
length: 2

The numbers are more manageable. Maybe be could put check, copy and size in their
own patches and put the remaining in a 4th one? (and I'd look closely if the
changes make sense based on the function name and content, means the const
addition has a "real" intent i.e not just a technical one).

> > Any other ideas?
> 
> Just that the most useful const additions (IMHO) are going to be
> places at the top of a big hierarchy, where callers expect something
> not to be modified, but the lowest levels are too far removed from
> that expectation for developers to easily remember.

I think that's a good idea, thanks!

> I imagine those
> cases are not going to be easy to find automatically (but that
> shouldn't discourage incremental improvements that can be found that
> way).

Yeah, will git it a try though.

Regards,

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