[Pgpool-hackers] removing backend_socket_dir parameter ?

[Guillaume Lelarge]

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.


-- 
Guillaume
 http://www.postgresql.fr
 http://dalibo.com