Обсуждение: Remove unused function parameters, part 1: contrib

Hi hackers,

PFA patches to remove unused function parameters in contrib (more patches will
come for non contrib).

The patches are split by modules to ease the review.

For 0005 (collid not used in internal_citext_pattern_cmp()) I think that the
unused column should be used (as done in 0005) due to an oversight in commit
f2464997644c.

This has been done with the help of a coccinelle script. It needs more polishing,
so will share later.

Regards,

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

On Thu, 27 Nov 2025 at 11:57, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi hackers,
>
> PFA patches to remove unused function parameters in contrib (more patches will
> come for non contrib).
>
> The patches are split by modules to ease the review.
>
> For 0005 (collid not used in internal_citext_pattern_cmp()) I think that the
> unused column should be used (as done in 0005) due to an oversight in commit
> f2464997644c.
>
> This has been done with the help of a coccinelle script. It needs more polishing,
> so will share later.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com

Hi!
Regarding 0005. collid arg was not used, and nobody ever compiled,
and then we want to make a change in citext_pattern_cmp behaviour. I
think we need to at least exercise this code in regression tests.

-- 
Best regards,
Kirill Reshke

Hi,

Thank you for working on this!

On Thu, 27 Nov 2025 at 11:57, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi hackers,
>
> PFA patches to remove unused function parameters in contrib (more patches will
> come for non contrib).
>
> The patches are split by modules to ease the review.

I applied all patches and built/tested without any errors. So I guess
that the changes are correct.

I skimmed through 0001-0003 to see if I can find more parameters to
delete and found that we can remove the 'tinfo' parameter from the
gbt_var_node_truncate() function.

> For 0005 (collid not used in internal_citext_pattern_cmp()) I think that the
> unused column should be used (as done in 0005) due to an oversight in commit
> f2464997644c.

I do not have a context on this but there is a comment in the
citextcmp() function, which might be related to this context:

    /*
     * We must do our str_tolower calls with DEFAULT_COLLATION_OID, not the
     * input collation as you might expect.  This is so that the behavior of
     * citext's equality and hashing functions is not collation-dependent.  We
     * should change this once the core infrastructure is able to cope with
     * collation-dependent equality and hashing functions.
     */

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On Thu, Nov 27, 2025 at 02:39:39PM +0300, Kirill Reshke wrote:
> On Thu, 27 Nov 2025 at 11:57, Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > For 0005 (collid not used in internal_citext_pattern_cmp()) I think that the
> > unused column should be used (as done in 0005) due to an oversight in commit
> > f2464997644c.
> >
> > This has been done with the help of a coccinelle script. It needs more polishing,
> > so will share later.
> >
> Hi!
> Regarding 0005. collid arg was not used, and nobody ever compiled,
> and then we want to make a change in citext_pattern_cmp behaviour. I
> think we need to at least exercise this code in regression tests.

Thanks for looking at it!

This function is already exercised with the tests added in f2464997644c so that
looks ok to me.

Maybe Jacob or Andrew have some thoughts to share?

Regards,

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

Hi,

On Thu, Nov 27, 2025 at 03:07:17PM +0300, Nazir Bilal Yavuz wrote:
> On Thu, 27 Nov 2025 at 11:57, Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > The patches are split by modules to ease the review.
> 
> I applied all patches and built/tested without any errors. So I guess
> that the changes are correct.

Thanks for looking at it!

> I skimmed through 0001-0003 to see if I can find more parameters to
> delete and found that we can remove the 'tinfo' parameter from the
> gbt_var_node_truncate() function.

Yeah, also found it with an improved version of the coccinelle script. Also
there is one more in postgres_fdw: unused rte parameter in create_foreign_modify()
is oversight in commit a61b1f74823.

And also one in pg_walinspect.

All added in the attached.

> > For 0005 (collid not used in internal_citext_pattern_cmp()) I think that the
> > unused column should be used (as done in 0005) due to an oversight in commit
> > f2464997644c.
> 
> I do not have a context on this but there is a comment in the
> citextcmp() function, which might be related to this context:
> 
>     /*
>      * We must do our str_tolower calls with DEFAULT_COLLATION_OID, not the
>      * input collation as you might expect.  This is so that the behavior of
>      * citext's equality and hashing functions is not collation-dependent.  We
>      * should change this once the core infrastructure is able to cope with
>      * collation-dependent equality and hashing functions.
>      */

Interesting... So maybe we should just remove collid in
internal_citext_pattern_cmp() instead: that's what 0005 is now doing.

Tha said, I'm not familiar with this code area, so better to wait other feedbacks
for 0005 then.

Regards,

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

On Thu, Nov 27, 2025 at 4:13 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> This function is already exercised with the tests added in f2464997644c so that
> looks ok to me.
>
> Maybe Jacob or Andrew have some thoughts to share?

Ooh, blast from the past! You've found my very first Postgres review.
I do not remember it at all. :D

Your v2-0005 looks more correct (and less confusing) to me. I'm not
qualified to comment on the historical behavior of citext+collations.

--Jacob