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

[Kwangwon Seo]

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.

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