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

[Tatsuo Ishii]

> Hello.
> 
> thanks for the fast reply with much informative things!!!
> 
> In short, I'll reimplement about it and submit patch again.
>
> here's detail below.
> 
> - about ipv4-mapped-ipv6 platform support
> I have found this one 
> https://forums.freebsd.org/threads/creating-a-ipv4-ipv6-socket-in-c.92530/
> In the thread, there's a link of freebsd source.
> https://github.com/freebsd/freebsd-src/blob/main/usr.bin/sockstat/sockstat.c#L432
> So, I think, what I did is not a recommanded way for freeBSD.
> wow, I didn't know about this. and you're right. I think It's unstable for freeBSD.
> And I also watch this https://www.pgpool.net/docs/latest/en/html/intro-whatis.html
> The link says, FreeBSD is obviously one of support architecture of pgpool.
> 
> - changing implementation direction.
> Oh I thought that there were some reason for that word what you said (need to fix only 4 function) But It seems not now!
> Then I can choose pgpool's way, as you recommanded.
> Yes. It will be a better way for product..!
> 
> - compiler warning of asprintf() usage
> Ohh, I have checked multiple times, but I miss that one..
> I'll handle that warning next time.
> 
> - replacement of asprintf()
> I have just checked the psprintf(). 
> Yes, I'm agree with your opinion. So I'll replace asprintf() to psprintf() way.
> 
> - regression tests
> Oh this is my bad. I just did one test, 004.watchdog in this time.
> Next time, I'll do regression test with ./regress.sh watchdog*. and also tests all regressions again before submit the patch.
> 
> - patch files
> Yes I got it!
> 
> 
> 
> whoa Thanks for the reviewing my patch. It helps me a lot!!
> 
> As I mentioned, I want to submit another patch So I'm working on it.


Thank you for understanding. Looking forward to reviewing your new
patch.

> If you want to say someting, you can say at ease.
>
> Best Regards...
> 
> 
> Kwangwon Seo
> 
> Bitnine Global Inc.
> www.bitnine.net
> 
> 
> On Saturday, July 13th, 2024 at 07:53, Tatsuo Ishii <ishii at postgresql.org> wrote:
> 
>> 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