> 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 _______________________________________________ Pgpool-hackers mailing list [email protected] http://pgfoundry.org/mailman/listinfo/pgpool-hackers
