On 05.01.26 17:18, Bertrand Drouvot wrote:
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].
I have committed the remaining two patches.
I had in my own work parked a few changes that were similar to the ones
in your patch 0001, so I threw them in there as well.
