Обсуждение: Don't cast away const where possible

Hi hackers,

Some functions are casting away the const qualifiers from their signatures in
local variables.

These 3 patches add const to read only local variables, preserving the const
qualifiers from the function signatures.

0001: those are simple changes in 6 files (16 changes in total)

0002: Add const to read only TableInfo pointers in pg_dump

Functions that dump table data receive their parameters through const void *
but were casting away const. Add const qualifiers to functions that only read
the table information.

Also change getRootTableInfo to return const TableInfo *, since it only traverses
the parent chain without modifying any TableInfo structures. This allows the dump
functions to maintain const correctness when calling it.

0003: Separate read and write pointers in pg_saslprep

Use separate pointers for reading const input ('p') and writing
to mutable output ('outp'), avoiding the need to cast away const on the input
parameter.

It has been done with the help of [1], but not all the changes proposed by it have
been implemented. Indeed, I did some filtering and decided not to change the ones
that:

- are just thin wrappers
- would require public API changes
- rely on external functions (such as LZ4F_compressUpdate())
- would require changes beyond the scope of this cleanup

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

Thoughts?

Regards,

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

On 18.12.25 14:55, Bertrand Drouvot wrote:
> Some functions are casting away the const qualifiers from their signatures in
> local variables.

@@ -1304,8 +1304,8 @@ merge_overlapping_ranges(FmgrInfo *cmp, Oid colloid,
  static int
  compare_distances(const void *a, const void *b)
  {
-       DistanceValue *da = (DistanceValue *) a;
-       DistanceValue *db = (DistanceValue *) b;
+       const DistanceValue *da = (const DistanceValue *) a;
+       const DistanceValue *db = (const DistanceValue *) b;

I wonder if the better fix here wouldn't be to get rid of the cast. 
It's not necessary, and without it the compiler would automatically warn 
about qualifier mismatches.  These comparison functions seem to be a 
common pattern.

Hi,

On Mon, Dec 22, 2025 at 12:53:03PM +0100, Peter Eisentraut wrote:
> On 18.12.25 14:55, Bertrand Drouvot wrote:
> > Some functions are casting away the const qualifiers from their signatures in
> > local variables.
> 
> @@ -1304,8 +1304,8 @@ merge_overlapping_ranges(FmgrInfo *cmp, Oid colloid,
>  static int
>  compare_distances(const void *a, const void *b)
>  {
> -       DistanceValue *da = (DistanceValue *) a;
> -       DistanceValue *db = (DistanceValue *) b;
> +       const DistanceValue *da = (const DistanceValue *) a;
> +       const DistanceValue *db = (const DistanceValue *) b;
> 
> I wonder if the better fix here wouldn't be to get rid of the cast. It's not
> necessary, and without it the compiler would automatically warn about
> qualifier mismatches.

Yeah, that looks better as it provides an extra safety check should the function
signature change.

> These comparison functions seem to be a common
> pattern.

Right, in the attached I applied your proposal on all those places.

Regards,

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

> On Dec 29, 2025, at 17:01, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Mon, Dec 22, 2025 at 12:53:03PM +0100, Peter Eisentraut wrote:
>> On 18.12.25 14:55, Bertrand Drouvot wrote:
>>> Some functions are casting away the const qualifiers from their signatures in
>>> local variables.
>>
>> @@ -1304,8 +1304,8 @@ merge_overlapping_ranges(FmgrInfo *cmp, Oid colloid,
>> static int
>> compare_distances(const void *a, const void *b)
>> {
>> -       DistanceValue *da = (DistanceValue *) a;
>> -       DistanceValue *db = (DistanceValue *) b;
>> +       const DistanceValue *da = (const DistanceValue *) a;
>> +       const DistanceValue *db = (const DistanceValue *) b;
>>
>> I wonder if the better fix here wouldn't be to get rid of the cast. It's not
>> necessary, and without it the compiler would automatically warn about
>> qualifier mismatches.
>
> Yeah, that looks better as it provides an extra safety check should the function
> signature change.
>
>> These comparison functions seem to be a common
>> pattern.
>
> Right, in the attached I applied your proposal on all those places.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>
<v2-0001-Don-t-cast-away-const-where-possible.patch><v2-0002-Add-const-to-read-only-TableInfo-pointers-in-pg_d.patch><v2-0003-Separate-read-and-write-pointers-in-pg_saslprep.patch>


I have similar patch at https://docs.qq.com/sheet/DR0JRQ3lPVGtCWW5q?tab=000001&_t=1761030496005&nlc=1 doing the exact
samething in pg_dump_sort.c. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On Mon, Dec 29, 2025 at 09:01:46AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Dec 22, 2025 at 12:53:03PM +0100, Peter Eisentraut wrote:
> > On 18.12.25 14:55, Bertrand Drouvot wrote:
> > > Some functions are casting away the const qualifiers from their signatures in
> > > local variables.
> > 
> > @@ -1304,8 +1304,8 @@ merge_overlapping_ranges(FmgrInfo *cmp, Oid colloid,
> >  static int
> >  compare_distances(const void *a, const void *b)
> >  {
> > -       DistanceValue *da = (DistanceValue *) a;
> > -       DistanceValue *db = (DistanceValue *) b;
> > +       const DistanceValue *da = (const DistanceValue *) a;
> > +       const DistanceValue *db = (const DistanceValue *) b;
> > 
> > I wonder if the better fix here wouldn't be to get rid of the cast. It's not
> > necessary, and without it the compiler would automatically warn about
> > qualifier mismatches.
> 
> Yeah, that looks better as it provides an extra safety check should the function
> signature change.

Out of curiosity, I searched for places where we could remove explicit casts when
assigning from void pointers (relying on implicit conversion instead), that would
lead to:

"
 157 files changed, 387 insertions(+), 388 deletions(-)
"

That's not a small patch and I think that doing this work is valuable though.

We could imagine, working on say 20 files at a time and say once per month.
That would ease the review(s) and also avoid too many rebases for patches waiting
in the commitfest.

Thoughts?

Regards,

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

On 29.12.25 10:01, Bertrand Drouvot wrote:
> On Mon, Dec 22, 2025 at 12:53:03PM +0100, Peter Eisentraut wrote:
>> On 18.12.25 14:55, Bertrand Drouvot wrote:
>>> Some functions are casting away the const qualifiers from their signatures in
>>> local variables.
>>
>> @@ -1304,8 +1304,8 @@ merge_overlapping_ranges(FmgrInfo *cmp, Oid colloid,
>>   static int
>>   compare_distances(const void *a, const void *b)
>>   {
>> -       DistanceValue *da = (DistanceValue *) a;
>> -       DistanceValue *db = (DistanceValue *) b;
>> +       const DistanceValue *da = (const DistanceValue *) a;
>> +       const DistanceValue *db = (const DistanceValue *) b;
>>
>> I wonder if the better fix here wouldn't be to get rid of the cast. It's not
>> necessary, and without it the compiler would automatically warn about
>> qualifier mismatches.
> 
> Yeah, that looks better as it provides an extra safety check should the function
> signature change.
> 
>> These comparison functions seem to be a common
>> pattern.
> 
> Right, in the attached I applied your proposal on all those places.

I have committed patch 0003 (pg_saslprep).

For patch 0002, I don't understand the change for getRootTableInfo(). 
It returns tbinfo->parents[0] (possibly some levels deep), but the 
parents field is not const-qualfied, so I don't understand the claim 
that this fixes anything.

For patch 0001, this seems good, but I wonder why your patch catches 
some cases and not some other similar ones.  For example, in 
src/backend/access/brin/brin_minmax_multi.c, you change 
compare_distances(), but not the very similar compare_expanded_ranges() 
and compare_values() nearby.

Hi,

On Mon, Jan 05, 2026 at 02:35:43PM +0100, Peter Eisentraut wrote:
> On 29.12.25 10:01, Bertrand Drouvot wrote:
> > Right, in the attached I applied your proposal on all those places.
> 
> I have committed patch 0003 (pg_saslprep).

Thanks!

> For patch 0002, I don't understand the change for getRootTableInfo(). It
> returns tbinfo->parents[0] (possibly some levels deep), but the parents
> field is not const-qualfied, so I don't understand the claim that this fixes
> anything.

You're right, the function doesn't modify anything that its argument's pointer
members point to. If it did, that would be misleading to accept a const parameter
while modifying any of its non const pointer members data. getRootTableInfo()
is not one of those cases so PFA a new version without the getRootTableInfo()
related changes.

> 
> For patch 0001, this seems good, but I wonder why your patch catches some
> cases and not some other similar ones.  For example, in
> src/backend/access/brin/brin_minmax_multi.c, you change compare_distances(),
> but not the very similar compare_expanded_ranges() and compare_values()
> nearby.

The initial patch was filtering out more complex functions that would need more
study. The idea was to look at those later on.

Now, about compare_expanded_ranges() and compare_values(), that's right that
those functions have similar patterns and could be included and their "extra"
study is simple as realizing that minval and maxval are Datum (so uint64_t),
are pass by values to FunctionCall2Coll() so that it can not modify them.

So, better to be consistent within the same file, those 2 functions have been
added in the attached.

Also I've added the changes for sort_item_compare() even this is a thin wrapper
so that the changes are consistent accross the mcv.c file too.

Now all the remaining ones reported by [1] are in files not touched by the attached,
making it consistent on a per file basis.

Note that it does not take care at all of "nearby" places where we could remove
explicit casts when assigning from void pointers (for example the arg parameter
in compare_expanded_ranges() and compare_values()) as I think that could be
worth a dedicated project as stated in [2].

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/search_const_away.cocci
[2]: https://www.postgresql.org/message-id/aVTiCHBalaFCneYD%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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