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
>From 541be8414a50520cd370e5aa546a4532f81bc3f7 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Thu, 18 Dec 2025 09:51:01 +0000 Subject: [PATCH v3 1/2] Don't cast away const where possible Add const to read only local variables, preserving the const qualifiers from the function signatures. This does not change all such instances, but only those hand-picked by the author. The ones that are not changed: - are just thin wrappers (except when there is other changes in the same file) - would require public API changes - rely on external functions (such as LZ4F_compressUpdate()) - would require complex subsystem changes Discussion: https://postgr.es/m/aUQHy/MmWq7c97wK%40ip-10-97-1-34.eu-west-3.compute.internal --- src/backend/access/brin/brin_minmax_multi.c | 12 ++++++------ src/backend/access/heap/pruneheap.c | 4 ++-- src/backend/access/spgist/spgkdtreeproc.c | 8 ++++---- src/backend/statistics/mcv.c | 8 ++++---- src/backend/tsearch/spell.c | 4 ++-- src/test/modules/injection_points/injection_points.c | 8 ++++---- 6 files changed, 22 insertions(+), 22 deletions(-) 22.0% src/backend/access/brin/ 9.9% src/backend/access/heap/ 14.9% src/backend/access/spgist/ 13.3% src/backend/statistics/ 9.2% src/backend/tsearch/ 30.5% src/test/modules/injection_points/ diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index 6b86b1fd889..75dd8d5083b 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -857,8 +857,8 @@ brin_range_deserialize(int maxvalues, SerializedRanges *serialized) static int compare_expanded_ranges(const void *a, const void *b, void *arg) { - ExpandedRange *ra = (ExpandedRange *) a; - ExpandedRange *rb = (ExpandedRange *) b; + const ExpandedRange *ra = a; + const ExpandedRange *rb = b; Datum r; compare_context *cxt = (compare_context *) arg; @@ -895,8 +895,8 @@ compare_expanded_ranges(const void *a, const void *b, void *arg) static int compare_values(const void *a, const void *b, void *arg) { - Datum *da = (Datum *) a; - Datum *db = (Datum *) b; + const Datum *da = a; + const Datum *db = b; Datum r; compare_context *cxt = (compare_context *) arg; @@ -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 = a; + const DistanceValue *db = b; if (da->value < db->value) return 1; diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index af788b29714..632c2427952 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -2021,8 +2021,8 @@ heap_log_freeze_eq(xlhp_freeze_plan *plan, HeapTupleFreeze *frz) static int heap_log_freeze_cmp(const void *arg1, const void *arg2) { - HeapTupleFreeze *frz1 = (HeapTupleFreeze *) arg1; - HeapTupleFreeze *frz2 = (HeapTupleFreeze *) arg2; + const HeapTupleFreeze *frz1 = arg1; + const HeapTupleFreeze *frz2 = arg2; if (frz1->xmax < frz2->xmax) return -1; diff --git a/src/backend/access/spgist/spgkdtreeproc.c b/src/backend/access/spgist/spgkdtreeproc.c index 6acda3fdcff..1ec0a4f59f3 100644 --- a/src/backend/access/spgist/spgkdtreeproc.c +++ b/src/backend/access/spgist/spgkdtreeproc.c @@ -84,8 +84,8 @@ typedef struct SortedPoint static int x_cmp(const void *a, const void *b) { - SortedPoint *pa = (SortedPoint *) a; - SortedPoint *pb = (SortedPoint *) b; + const SortedPoint *pa = a; + const SortedPoint *pb = b; if (pa->p->x == pb->p->x) return 0; @@ -95,8 +95,8 @@ x_cmp(const void *a, const void *b) static int y_cmp(const void *a, const void *b) { - SortedPoint *pa = (SortedPoint *) a; - SortedPoint *pb = (SortedPoint *) b; + const SortedPoint *pa = a; + const SortedPoint *pb = b; if (pa->p->y == pb->p->y) return 0; diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c index 390ad83497a..9749871b18e 100644 --- a/src/backend/statistics/mcv.c +++ b/src/backend/statistics/mcv.c @@ -402,8 +402,8 @@ count_distinct_groups(int numrows, SortItem *items, MultiSortSupport mss) static int compare_sort_item_count(const void *a, const void *b, void *arg) { - SortItem *ia = (SortItem *) a; - SortItem *ib = (SortItem *) b; + const SortItem *ia = a; + const SortItem *ib = b; if (ia->count == ib->count) return 0; @@ -465,8 +465,8 @@ static int sort_item_compare(const void *a, const void *b, void *arg) { SortSupport ssup = (SortSupport) arg; - SortItem *ia = (SortItem *) a; - SortItem *ib = (SortItem *) b; + const SortItem *ia = a; + const SortItem *ib = b; return ApplySortComparator(ia->values[0], ia->isnull[0], ib->values[0], ib->isnull[0], diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c index 2e96c86b0b3..e3436dbddd2 100644 --- a/src/backend/tsearch/spell.c +++ b/src/backend/tsearch/spell.c @@ -210,8 +210,8 @@ cmpspellaffix(const void *s1, const void *s2) static int cmpcmdflag(const void *f1, const void *f2) { - CompoundAffixFlag *fv1 = (CompoundAffixFlag *) f1, - *fv2 = (CompoundAffixFlag *) f2; + const CompoundAffixFlag *fv1 = f1; + const CompoundAffixFlag *fv2 = f2; Assert(fv1->flagMode == fv2->flagMode); diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index 9c3806f5958..17d95caf164 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -189,7 +189,7 @@ injection_init_shmem(void) * otherwise. */ static bool -injection_point_allowed(InjectionPointCondition *condition) +injection_point_allowed(const InjectionPointCondition *condition) { bool result = true; @@ -232,7 +232,7 @@ injection_points_cleanup(int code, Datum arg) void injection_error(const char *name, const void *private_data, void *arg) { - InjectionPointCondition *condition = (InjectionPointCondition *) private_data; + const InjectionPointCondition *condition = private_data; char *argstr = (char *) arg; if (!injection_point_allowed(condition)) @@ -248,7 +248,7 @@ injection_error(const char *name, const void *private_data, void *arg) void injection_notice(const char *name, const void *private_data, void *arg) { - InjectionPointCondition *condition = (InjectionPointCondition *) private_data; + const InjectionPointCondition *condition = private_data; char *argstr = (char *) arg; if (!injection_point_allowed(condition)) @@ -268,7 +268,7 @@ injection_wait(const char *name, const void *private_data, void *arg) uint32 old_wait_counts = 0; int index = -1; uint32 injection_wait_event = 0; - InjectionPointCondition *condition = (InjectionPointCondition *) private_data; + const InjectionPointCondition *condition = private_data; if (inj_state == NULL) injection_init_shmem(); -- 2.34.1
>From ffde17e8b053209a17b6776f58e994b90483ba8e Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Thu, 18 Dec 2025 12:47:07 +0000 Subject: [PATCH v3 2/2] 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. Discussion: https://postgr.es/m/aUQHy/MmWq7c97wK%40ip-10-97-1-34.eu-west-3.compute.internal --- src/bin/pg_dump/pg_dump.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 100.0% src/bin/pg_dump/ diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 7df56d8b1b0..e3b4035e57a 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -2379,8 +2379,8 @@ selectDumpableObject(DumpableObject *dobj, Archive *fout) static int dumpTableData_copy(Archive *fout, const void *dcontext) { - TableDataInfo *tdinfo = (TableDataInfo *) dcontext; - TableInfo *tbinfo = tdinfo->tdtable; + const TableDataInfo *tdinfo = dcontext; + const TableInfo *tbinfo = tdinfo->tdtable; const char *classname = tbinfo->dobj.name; PQExpBuffer q = createPQExpBuffer(); @@ -2547,8 +2547,8 @@ dumpTableData_copy(Archive *fout, const void *dcontext) static int dumpTableData_insert(Archive *fout, const void *dcontext) { - TableDataInfo *tdinfo = (TableDataInfo *) dcontext; - TableInfo *tbinfo = tdinfo->tdtable; + const TableDataInfo *tdinfo = dcontext; + const TableInfo *tbinfo = tdinfo->tdtable; DumpOptions *dopt = fout->dopt; PQExpBuffer q = createPQExpBuffer(); PQExpBuffer insertStmt = NULL; @@ -2618,7 +2618,7 @@ dumpTableData_insert(Archive *fout, const void *dcontext) */ if (insertStmt == NULL) { - TableInfo *targettab; + const TableInfo *targettab; insertStmt = createPQExpBuffer(); @@ -2870,7 +2870,7 @@ static void dumpTableData(Archive *fout, const TableDataInfo *tdinfo) { DumpOptions *dopt = fout->dopt; - TableInfo *tbinfo = tdinfo->tdtable; + const TableInfo *tbinfo = tdinfo->tdtable; PQExpBuffer copyBuf = createPQExpBuffer(); PQExpBuffer clistBuf = createPQExpBuffer(); DataDumperPtr dumpFn; @@ -2891,7 +2891,7 @@ dumpTableData(Archive *fout, const TableDataInfo *tdinfo) (dopt->load_via_partition_root || forcePartitionRootLoad(tbinfo))) { - TableInfo *parentTbinfo; + const TableInfo *parentTbinfo; char *sanitized; parentTbinfo = getRootTableInfo(tbinfo); -- 2.34.1
