Обсуждение: some unused parameters cleanup

Here is a series of patches to remove some unused function parameters. 
In each case, the need for them was removed by some other code changes 
over time but the unusedness was not noticed.  I have included a 
reference to when they became unused in each case.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Here is a series of patches to remove some unused function parameters. 
> In each case, the need for them was removed by some other code changes 
> over time but the unusedness was not noticed.  I have included a 
> reference to when they became unused in each case.

For some of these, there's an argument for keeping the unused parameter
for consistency with sibling functions that do use it.  Not sure how
important that is, though.

            regards, tom lane

On Tue, Aug 25, 2020 at 12:59:31PM -0400, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > Here is a series of patches to remove some unused function parameters. 
> > In each case, the need for them was removed by some other code changes 
> > over time but the unusedness was not noticed.  I have included a 
> > reference to when they became unused in each case.
> 
> For some of these, there's an argument for keeping the unused parameter
> for consistency with sibling functions that do use it.  Not sure how
> important that is, though.

I think if they are kept for that reason, we should document that so we
know not to revisit this issue for them.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

On 8/25/20 7:42 PM, Bruce Momjian wrote:
> On Tue, Aug 25, 2020 at 12:59:31PM -0400, Tom Lane wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>> Here is a series of patches to remove some unused function parameters.
>>> In each case, the need for them was removed by some other code changes
>>> over time but the unusedness was not noticed.  I have included a
>>> reference to when they became unused in each case.
>>
>> For some of these, there's an argument for keeping the unused parameter
>> for consistency with sibling functions that do use it.  Not sure how
>> important that is, though.
> 
> I think if they are kept for that reason, we should document that so we
> know not to revisit this issue for them.

+1

That way we can avoid new people discovering the same unused parameters 
and then submitting patches for them.

Andreas

On 2020/08/26 2:50, Andreas Karlsson wrote:
> On 8/25/20 7:42 PM, Bruce Momjian wrote:
>> On Tue, Aug 25, 2020 at 12:59:31PM -0400, Tom Lane wrote:
>>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>>> Here is a series of patches to remove some unused function parameters.
>>>> In each case, the need for them was removed by some other code changes
>>>> over time but the unusedness was not noticed.  I have included a
>>>> reference to when they became unused in each case.
>>>
>>> For some of these, there's an argument for keeping the unused parameter
>>> for consistency with sibling functions that do use it.  Not sure how
>>> important that is, though.
>>
>> I think if they are kept for that reason, we should document that so we
>> know not to revisit this issue for them.> 
> +1
> 
> That way we can avoid new people discovering the same unused parameters and then submitting patches for them.

I agree that some parameters were kept for that reason,
but ISTM that also some were kept just accidentally.
For example, regarding unused parameter "encoding" that 0010 patch
tries to remove, commit f0d6f20278 got rid of the use of "encoding"
from generate_normalized_query() but ISTM that it just forgot to
remove that parameter.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020-08-25 18:59, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> Here is a series of patches to remove some unused function parameters.
>> In each case, the need for them was removed by some other code changes
>> over time but the unusedness was not noticed.  I have included a
>> reference to when they became unused in each case.
> 
> For some of these, there's an argument for keeping the unused parameter
> for consistency with sibling functions that do use it.  Not sure how
> important that is, though.

I had meant to exclude cases like this from this patch set.  If you see 
a case like this in *this* patch set, please point it out.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Aug 26, 2020 at 06:38:52AM +0200, Peter Eisentraut wrote:
> I had meant to exclude cases like this from this patch set.  If you see a
> case like this in *this* patch set, please point it out.

Last time I looked at that a lot of parameters are kept around as a
matter of symmetry with siblings, like tablecmds.c.  FWIW:
https://www.postgresql.org/message-id/20190130073317.GP3121@paquier.xyz

Saying that, I can see that you have been careful here and I don't see
anything like that in most of the changes you are proposing here.  You
could say that for findNamespace() or _moveBefore() perhaps, but there
are also some routines not making use of an Archive.  So this cleanup
looks fine to me.
--
Michael

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2020-08-25 18:59, Tom Lane wrote:
>> For some of these, there's an argument for keeping the unused parameter
>> for consistency with sibling functions that do use it.  Not sure how
>> important that is, though.

> I had meant to exclude cases like this from this patch set.  If you see 
> a case like this in *this* patch set, please point it out.

I'd been thinking specifically of the changes in pg_backup_archiver.c.
But now that I look around a bit further, there's already very little
consistency in that file about whether to pass the ArchiveHandle* pointer
everywhere.  So no further objection here.

            regards, tom lane