Thread: 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>
Attachment
> 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>
Attachment
> 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>