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

[Gilles Darold]

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

> And thanks Gilles for this second patch. I should really start working
> on my own :)

Thanks a lot for your patch review and improvement.

-- 
Gilles Darold
http://dalibo.com - http://dalibo.org