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

Mon Aug 5 20:51:34 JST 2024

Thank you for the patch! I will look into it.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

> Hello it's been a long time.
> 
> I just came back with patch files again.
> 
> I hope not too late. (sorry actually a little bit late...)
> 
> ## regression tests
> 
> Several tests are conducted with various configuration. hostname is came from /etc/hosts.
> 
> 1. ipv4 test(127.0.0.1, localhost)
> 2. ipv6 test(::1, ip6-localhost)
> 3. combined ipv4 and ipv6 test(node0 : given ipv4, node1 and node2 : given ipv6)
> 
> target: 004, 011, 012, 013, 014, 015
> 
> ## pg_indent
> 
> pg_indent is applied to only my changes, not to other existing one.
> 
> ## p.s.
> 
> If you review this patch, I would be happy!
> 
> Any kind of suggestion, criticism welcome.
> 
> This patch is a proposal, so you can reject this or request some additional things to me.
> 
> ---
> 
> Regards
> Kwangwon Seo
> 
> Bitnine Global Inc.
> www.bitnine.net
> 
> 
> On Saturday, July 13th, 2024 at 8:31 PM, Tatsuo Ishii <ishii at postgresql.org> wrote:
> 
>> > 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