[Pgpool-hackers] removing backend_socket_dir parameter ?

Tue Jan 11 01:52:37 UTC 2011

> Le 07/01/2011 11:01, Guillaume Lelarge a écrit :
>> Le 07/01/2011 10:59, Tatsuo Ishii a écrit :
>>>> Le 07/01/2011 01:55, Tatsuo Ishii a écrit :
>>>>>> Le 06/01/2011 15:43, Jehan-Guillaume (ioguix) de Rorthais a écrit :
>>>>>>> Le 05/01/2011 10:51, Jehan-Guillaume (ioguix) de Rorthais a écrit :
>>>>>>>> Le 05/01/2011 10:22, Tatsuo Ishii a écrit :
>>>>>>>>>> Le 04/01/2011 15:38, Jehan-Guillaume (ioguix) de Rorthais a écrit :
>>>>>>>>>>> Le 04/01/2011 15:13, Guillaume Lelarge a écrit :
>>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>>> Le 04/01/2011 14:27, Jehan-Guillaume (ioguix) de Rorthais a écrit :
>>>>>>>>>>>>> [...]
>>>>>>>>>>>>> pgPool is currently using the parameter "backend_socket_dir" to define where it
>>>>>>>>>>>>> can find the backend unix socket files.
>>>>>>>>>>>>>
>>>>>>>>>>>>> However, from the libpq world, we just use one parameter for both unix or inet
>>>>>>>>>>>>> socket: host. If this parameter starts with '/', then this is a unix socket. All
>>>>>>>>>>>>> other values are inet connections. See:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://www.postgresql.org/docs/9.0/static/libpq-connect.html#LIBPQ-CONNECT-HOST
>>>>>>>>>>>>>
>>>>>>>>>>>>> Back in pgPool world, the equivalent parameter is "backend_hostnameN". So, when
>>>>>>>>>>>>> I was setting up a dev environment yesterday, I naturally set this parameter  to
>>>>>>>>>>>>> my unix path (I'm using Debian) and end up with a hostname resolution error.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So you'll find in attachment a patch to remove the "backend_socket_dir"
>>>>>>>>>>>>> parameter and use the libpq policy. Moreover, on empty "backend_hostnameN"
>>>>>>>>>>>>> value, the patch fall back to DEFAULT_SOCKET_DIR.
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> This patch is really interesting. This is something I had on my todo
>>>>>>>>>>>> list for quite some time.
>>>>>>>>>>>
>>>>>>>>>>> Glad to hear that :)
>>>>>>>>>>> I wasn't the only one wondering about this then :)
>>>>>>>>>>>
>>>>>>>>>>>>> I realize it will break compatibility with older configuration file. But in a
>>>>>>>>>>>>> first step, I prefer something clean and start discussing this issue with you if
>>>>>>>>>>>>> you really mind this.
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> On the configuration file, compatibility of configuration file is never
>>>>>>>>>>>> garantied. So I don't think this is such an issue. We need to make it
>>>>>>>>>>>> clear that the parameter has a new mail.
>>>>>>>>>>>
>>>>>>>>>>> s/new mail/new name/
>>>>>>>>>>>
>>>>>>>>>>> well, it's not a new name, we just drop one and promote an existing one to deal
>>>>>>>>>>> with both unix and inet socket :)
>>>>>>>>>>>
>>>>>>>>>>>> If we really want to maintain the compatibility on this issue, we could
>>>>>>>>>>>> still support the old parameter during two or three major releases. I
>>>>>>>>>>>> guess we need to put a warning in the log to say "hey guy, you're still
>>>>>>>>>>>> using that old obsolete configuration parameter, you should better
>>>>>>>>>>>> change with this one".
>>>>>>>>>>>
>>>>>>>>>>> Yeah. What I have in mind is :
>>>>>>>>>>>   if backend_hostnameN == ''
>>>>>>>>>>>     if not backend_socket_dir
>>>>>>>>>>>       backend_hostnameN = DEFAULT_SOCKET_DIR
>>>>>>>>>>>     else
>>>>>>>>>>>       log « please stop using that »
>>>>>>>>>>>       backend_hostnameN = backend_socket_dir
>>>>>>>>>>>
>>>>>>>>>>> But really, I would prefer to drop backend_socket_dir and keep the code clean.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> You can't have it both ways. Either you please your users, or you please
>>>>>>>>>> your developpers. I think we should do the former.
>>>>>>>
>>>>>>>>> Agreed. I learned it in a hard way:-)
>>>>>>>
>>>>>>>>> Now I think the idea is great and am looking forward to accepting for
>>>>>>>>> the next release.
>>>>>>>
>>>>>>>> Thank you !
>>>>>>>
>>>>>>> So here is the new patch including backward compatibility with backend_socket_dir.
>>>>>>>
>>>>>>
>>>>>> Seems good to me. Could you also get rid of backend_socket_dir in the
>>>>>> sample configuration files? this parameter won't be the prefered way to
>>>>>> set the socket, so it shouldn't be in the sample conf files.
>>>>>
>>>>> But according to the patches still backend_socket_dir remains and even
>>>>> appears on show pool_status.
>>>>
>>>> Yes, I kept it because cause you agree'd Guillaume statement:
>>>> «
>>>> You can't have it both ways. Either you please your users, or you please your
>>>> developpers. I think we should do the former.
>>>> »
>>>>
>>>> My anderstanding is that "doing the former", so "please your users", means
>>>> keeping backward compatibility for some time. Are we ok ?
>>>
>>> Yup.
>>>
>>>> So my last patch implement the logic I described above:
>>>>
>>>>   if (backend_hostnameN == '' and backend_socket_dir != '')
>>>>     log "this parameter is deprecated ! use backend_socket_dir instead."
>>>>     backend_hostnameN = backend_socket_dir
>>>>
>>>>
>>>> So yes, it appears in show pool_status :(
>>>>
>>>>> Probably we should leave it in the
>>>>> pgpool.conf.sample etc. with statting that "this directive is
>>>>> obsoleted and used for only internal use." or something like that?
>>>>
>>>> We don't use it internally.
>>>
>>> Oh ok.
>>>
>>>> In my opinion, we should
>>>>   * remove it from the sample
>>>>   * warn the user if he still use it in its configuration file
>>>>   * add "DEPCRECATED use backend_hostname instead" as description for this
>>>> parameter in "show pool_status"
>>>
>>> If you and Guillaume agreed on this, I'm fine with removing it from
>>> the sample conf.
>> 
>> Yeah, I think with these three items.
> 
> So here is the patch.
> 
> I added a check in pool_process_reporting to hide backend_socket_dir if it is
> not set in the conf file.
> 
> I'm waiting for the feedback to patch the doc.

Looks good.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp