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

[Gilles Darold]

Hi,

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.

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.

Regards,

Le 26/12/2010 04:30, Tatsuo Ishii a écrit :
>> Here are some comments after trying this patch.
>>
>> First, I splitted the patch in two patches, so to have one patch per
>> functionality. You'll find the first one, syslog, attached.
>>
>> Le 21/12/2010 15:11, Gilles Darold a écrit :
>>> [...]
>>> Syslog patch :
>>>
>>>     It adds two configuration directives : "logsyslog = true|false" and
>>>     "logfacility = 'LOCAL0'"
>>>
>> I would much prefer to have the same name than PostgreSQL options. No
>> logsyslog, but a log_destination which accepts stderr and syslog. No
>> logfacility, but syslog_facility.
> Me too.
>
>>>     If 'logsyslog' is enabled, messages are sent to syslog (through
>>>     pool_error.c) just after the call of pool_get_config() in main.c.
>>>
>>>     Syslog ident it hard coded to "pgpool".
>>>
>> It shouldn't be hardcoded. I would better have a syslog_ident parameter.
> Right.
>
>>>     Connection to syslog is closed from pool_shmem_exit() in
>>>     pool_shmem.c because this is the only method always called at exit.
>>>     A better place should be in main.c but it has to be repeated so many
>>>     time so that I choose this lazy place :-)
>>>
>> Seems enough for me.
> I think so too.
>
>>>     Samples configuration files has been patched too.
>>>
>> Apart from the options name, and the missing option, I have a big issue
>> with the FACILITY variable. Where do you initialize it? AFAICT, nowhere.
>>
>> BTW, there is no reason to set only INIT_CONFIG for the facility
>> parameter. I should be INIT_CONFIG|RELOAD_CONFIG.
>>
>> So, all in all, I like this patch but it still needs some work. Gilles,
>> could you work on the few issues I talked about? and send another patch
>> (and this time only on syslog). If you can't, I'll do it.
>>
>> Now, I'll work on your second patch.
>>
>> Thanks.
>>
>>
>> -- 
>> Guillaume
>>  http://www.postgresql.fr
>>  http://dalibo.com


-- 
Gilles Darold
Administrateur de bases de données
http://dalibo.com - http://dalibo.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pgpool-II-patch_syslog.diff
Type: text/x-patch
Size: 14650 bytes
Desc: not available
URL: <http://pgfoundry.org/pipermail/pgpool-hackers/attachments/20101227/c98f06b9/attachment.bin>