Thread

1. Mark function arguments of type "T *" as "const T *" where possible

   Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-09T16:20:57Z

   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
   

2. Re: Mark function arguments of type "T *" as "const T *" where possible

   Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-10T06:39:40Z

   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
   
   
   
   

3. Re: Mark function arguments of type "T *" as "const T *" where possible

   Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-10T17:44:26Z

   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
   

4. Re: Mark function arguments of type "T *" as "const T *" where possible

   Jacob Champion <jacob.champion@enterprisedb.com> — 2025-12-10T20:22:22Z

   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
   
   
   
   

5. Re: Mark function arguments of type "T *" as "const T *" where possible

   Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-11T07:22:35Z

   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
   
   
   
   

6. Re: Mark function arguments of type "T *" as "const T *" where possible

   Jacob Champion <jacob.champion@enterprisedb.com> — 2025-12-11T17:51:49Z

   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
   
   
   
   

7. Re: Mark function arguments of type "T *" as "const T *" where possible

   Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-12T14:33:39Z

   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