[pgpool-hackers: 4483] Re: Watchdog and IPv6

[Tatsuo Ishii]

Hi Kwangwon,

Thank you for the patch!

> Oh Hi.. It's been a week...
> 
> I think I'm too slow! sorry about that.

No problem. Doing this work in a week is excellent in my opinion.

> Anyway, Here's the details and patch files.
> 
> - Why chose ipv4-mapped-ipv6 method
> Initially, I want to open multiple sockets for ipv4 socket and ipv6 socket just like pgpool process.
> but If I do so, I also have to modify struct SocketConnection. because it has one socket descriptor.
> That means, the requirements; "we need to fix only 4 functions" is not met.

Sorry, this is my bad. I did not intend to limit the number of
functions you need to modify. So this is not "requirements".

> So I chose ipv4-mapped-ipv6 method.

I am new to ipv4-mapped-ipv6 method. So I am not sure but am afraid
this method is not supported on some plaforms like FreeBSD?

https://stackoverflow.com/questions/2693709/what-was-the-motivation-for-adding-the-ipv6-v6only-flag
> Platform support
> Some platforms do not support dualstack sockets, either due to age or deliberate choice.
> For example, FreeBSD does not support IPv4-mapped addresses at all:

If this is true, better to return to the your original idea?

> Initially, I want to open multiple sockets for ipv4 socket and ipv6 socket just like pgpool process.

> - Use of asprintf
> asprintf() is a GNU extension. not a posix, or C standard.
> reference: https://www.gnu.org/software/gnulib/manual/html_node/asprintf.html
> But I think it's still good to use. also, there are some usage of asprintf() in the codes.

Thanks for pointing it out. Yes, I agree asprintf is useful. But
there's better alternative in pgpool: psprintf(imported from
PostgreSQL). It returns palloced memory (in pgpool backend
process). So you can either explicitly free the memory allocated by
psprintf by using pfree or free the memory altogether in the same
memory context using MemoryContextDelete.

BTW, I get some compiler warnings regarding asprintf

watchdog.c: In function ‘wd_create_recv_socket’:
watchdog.c:867:2: warning: ignoring return value of ‘asprintf’, declared with attribute warn_unused_result [-Wunused-result]
  867 |  asprintf(&portstr, "%d", port);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


I have a plan to replace all asprintf call to psprintf (PostgreSQL did
that long time ago).

> - Regression tests
> I tested 004.watchdog with changing hostnames with all branches which is in range of EOL.

I think you should test at least all *watchdog* tests.  I am getting
timeout error on 028.watchdog_enable_consensus_with_half_votes test.

> - etc
> I also know that quality is the most important thing.
> So, If I do something wrong, you can reject my patch or request to me to do something..!
> 
> - patch files(attachment)
> for V4_1, V4_2, V4_3, V4_4, V4_5, master

Since this is a new feature, you don't need to create patches other
than master because the patch will be pushed to master branch only.

> Best Regards...
>
> 
> Kwangwon Seo
> 
> Bitnine Global Inc.
> www.bitnine.net
> 
> 
> On Thursday, July 4th, 2024 at 16:30, Tatsuo Ishii <ishii at postgresql.org> wrote:
> 
>> > Oh... Can I challenge to that feature implementatation?
>> > 
>> > I saw that things ago, todo list and github issue also.
>> > 
>> > And I just saw that you updated about that one.
>> > 
>> > I was hesitated because I think that is already done or on progress by other developer.
>> 
>> 
>> As far as I know no other developers are working on this.
>> 
>> > If available, can I do this?
>> 
>> 
>> Sure. Why not. I will volunteer on reviewing your patch.
>> 
>> Best reagards,
>> --
>> Tatsuo Ishii
>> SRA OSS LLC
>> English: http://www.sraoss.co.jp/index_en/
>> Japanese:http://www.sraoss.co.jp