[Pgpool-hackers] [hackers] PgPool II patches : regex and syslog

[Guillaume Lelarge]

Le 28/12/2010 23:40, Tatsuo Ishii a écrit :
>> Le 28/12/2010 01:53, Tatsuo Ishii a écrit :
>>>> Le 27/12/2010 15:59, Guillaume Lelarge a écrit :
>>>>> Hi,
>>>>>
>>>>> Le 27/12/2010 10:47, Gilles Darold a écrit :
>>>>>> [...]
>>>>>> Here is the rewritten patch that add syslog support to PgPool with all
>>>>>> your great suggestions applied.
>>>>>> This patch has been created from the current cvs HEAD branch.
>>>>> Thanks. There is a bug in it, that I fixed in the attached patch. Even
>>>>> if you declare log_destination and syslog_ident as reloadable, it was
>>>>> without effect because there was no code to handle opening or closing
>>>>> syslog if they changed.
>>>> Damn you're right, I just simply omit this part. Sorry for the
>>>> additional work.
>>>>> There is another thing that annoys me, but it isn't a bug. What is the
>>>>> purpose of pool_config->logsyslog? is it just to avoid a strcmp? which
>>>>> seems a poor optimization wrt having two variables with same meaning.
>>>> No, this is a flag to delay PgPool logging to syslog only after the end
>>>> of configuration file reading. The problem appears when PgPool is run in
>>>> debug mode, during the configuration file it detect that log_destination
>>>> is set to syslog so if you don't wait (or use a flag)  it will try to
>>>> write to syslog before the syslog connection is opened.
>>>>
>>>>>> This patch also modify the comment about logdir configuraion directive.
>>>>>>
>>>>>> "Logging directory" has been change into "PgPool status file logging
>>>>>> directory"
>>>>>>
>>>>>> Feel free to apply or remove it. It just seems to me that 'logdir' =>
>>>>>> "Logging directory" is very confusing and false as this directive is
>>>>>> only used to save the pgpool_status file. It will certainly be better to
>>>>>> rename this directive, but for the moment just changing the comment help
>>>>>> a lot for understanding and preserve backward compatibility.
>>>>> I completely agree with you on that change. But I would much prefer to
>>>>> see it applied as another patch rather than in this patch. They have two
>>>>> different purposes, so they shouldn't be mixed.
>>>> Ok, memorized: one purpose per patch. Please remove it and make an other
>>>> one if you think it should be applied.
>>>>
>>>>> BTW, Tatsuo-san, Gilles and I have absolutely no idea if we should
>>>>> change pool_config.c, pool_config.l, or both. We did both, but I would
>>>>> really like to know which of these three possibilities is the good one.
>>>>> Thanks.
>>>> Yes this is because in cvs pool_config.c has not been fully regenerated
>>>> from pool_config.l and some part of the regex patch have not been
>>>> applied to pool_config.c
>>>
>>> I think you only need patches against pool_config.l. I will generate
>>> pool_config.c from pool_config.l.
>>>
>>
>> Do you need another patch from us, or is the current enough for you?
> 
> No, your patches look good. I will commit as soon as possible.

Thanks a lot.


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