Обсуждение: Rename setup_cancel_handler in pg_dump

Hi,

Attached is a simple patch to rename setup_cancel_handler()
in pg_dump/parallel.c. 

I am proposing it because there is a public function with
the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
does not include fe_utils/cancel.h, so there is no conflict,
but I think it is better to use different names to reduce
possible confusion. 

I guess there was no concerns when setup_cancel_handler in
pg_dump/parallel.c was introduced because the same name
function was not in fe_utils that could be used in common
between client tools.. The public setup_cancel_handler in
fe_utils was introduced in a4fd3aa719e, where this function
was moved from psql.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

> On 26 Jan 2024, at 01:42, Yugo NAGATA <nagata@sraoss.co.jp> wrote:

> I am proposing it because there is a public function with
> the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
> does not include fe_utils/cancel.h, so there is no conflict,
> but I think it is better to use different names to reduce
> possible confusion.

Given that a "git grep setup_cancel_hander" returns hits in pg_dump along with
other frontend utils, I can see the risk of confusion.

-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)

We don't have any other functions prefixed with pg_dump_, based on the naming
of the surrounding code in the file I wonder if set_cancel_handler is a more
appropriate name?

--
Daniel Gustafsson

On Tue, 30 Jan 2024 13:44:28 +0100
Daniel Gustafsson <daniel@yesql.se> wrote:

> > On 26 Jan 2024, at 01:42, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> 
> > I am proposing it because there is a public function with
> > the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
> > does not include fe_utils/cancel.h, so there is no conflict,
> > but I think it is better to use different names to reduce
> > possible confusion. 
> 
> Given that a "git grep setup_cancel_hander" returns hits in pg_dump along with
> other frontend utils, I can see the risk of confusion.

Thank you for looking into it!
 
> -setup_cancel_handler(void)
> +pg_dump_setup_cancel_handler(void)
> 
> We don't have any other functions prefixed with pg_dump_, based on the naming
> of the surrounding code in the file I wonder if set_cancel_handler is a more
> appropriate name?

Agreed. Here is a updated patch.

Regards,
Yugo Nagata

> 
> --
> Daniel Gustafsson
> 


-- 
Yugo NAGATA <nagata@sraoss.co.jp>

> On 1 Feb 2024, at 02:21, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> On Tue, 30 Jan 2024 13:44:28 +0100
> Daniel Gustafsson <daniel@yesql.se> wrote:

>> -setup_cancel_handler(void)
>> +pg_dump_setup_cancel_handler(void)
>>
>> We don't have any other functions prefixed with pg_dump_, based on the naming
>> of the surrounding code in the file I wonder if set_cancel_handler is a more
>> appropriate name?
>
> Agreed. Here is a updated patch.

Sleeping on it I still think this is a good idea, and hearing no objections I
went ahead with this.  Thanks for the patch!

--
Daniel Gustafsson

On Wed, 7 Feb 2024 22:59:48 +0100
Daniel Gustafsson <daniel@yesql.se> wrote:

> > On 1 Feb 2024, at 02:21, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> > On Tue, 30 Jan 2024 13:44:28 +0100
> > Daniel Gustafsson <daniel@yesql.se> wrote:
> 
> >> -setup_cancel_handler(void)
> >> +pg_dump_setup_cancel_handler(void)
> >> 
> >> We don't have any other functions prefixed with pg_dump_, based on the naming
> >> of the surrounding code in the file I wonder if set_cancel_handler is a more
> >> appropriate name?
> > 
> > Agreed. Here is a updated patch.
> 
> Sleeping on it I still think this is a good idea, and hearing no objections I
> went ahead with this.  Thanks for the patch!

Thank you!

Regards,
Yugo Nagata

> 
> --
> Daniel Gustafsson
> 


-- 
Yugo NAGATA <nagata@sraoss.co.jp>