Thread: Plug-in common/logging.h with vacuumlo and oid2name
Hi all, While refactoring some code, I have bumped into $subject, which causes both tools to have incorrect error message formats and progname being used all through the code, sometimes incorrectly or just missing. At the same time, we are missing a call to set_pglocale_pgservice() for both binaries. Any objections to the cleanup attached? -- Michael
Attachment
On 2019-08-20 03:28, Michael Paquier wrote:
> + pg_log_error("\nfailed to remove lo %u: %s", lo,
> + PQerrorMessage(conn));
This newline should be removed.
progname can probably be made into a local variable now.
The rest looks good to me.
Do we need set_pglocale_pgservice() calls here if we're not doing NLS?
Does the logging stuff require it? I'm not sure.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Sep 04, 2019 at 10:17:57AM +0200, Peter Eisentraut wrote:
> On 2019-08-20 03:28, Michael Paquier wrote:
> > + pg_log_error("\nfailed to remove lo %u: %s", lo,
> > + PQerrorMessage(conn));
>
> This newline should be removed.
Thanks, missed that.
> progname can probably be made into a local variable now.
Can it? vacuumlo() still uses the progname from _param for the
connection string.
> Do we need set_pglocale_pgservice() calls here if we're not doing NLS?
> Does the logging stuff require it? I'm not sure.
The logging part does not require it, but this can be used for
PGSYSCONFDIR, no?
--
Michael
Attachment
On 2019-09-04 14:17, Michael Paquier wrote: >> progname can probably be made into a local variable now. > > Can it? vacuumlo() still uses the progname from _param for the > connection string. Yeah, probably best to leave it as is for now. >> Do we need set_pglocale_pgservice() calls here if we're not doing NLS? >> Does the logging stuff require it? I'm not sure. > > The logging part does not require it, but this can be used for > PGSYSCONFDIR, no? How does PGSYSCONFDIR come into play here? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Sep 04, 2019 at 02:37:12PM +0200, Peter Eisentraut wrote: >>> Do we need set_pglocale_pgservice() calls here if we're not doing NLS? >>> Does the logging stuff require it? I'm not sure. >> >> The logging part does not require it, but this can be used for >> PGSYSCONFDIR, no? > > How does PGSYSCONFDIR come into play here? There is an argument to allow libpq to find out a service file for a connection from the executable path. Note that oid2name can use a connection string for connection, but not vacuumlo, so I somewhat missed that. -- Michael
Attachment
On Thu, Sep 05, 2019 at 08:59:41AM +0900, Michael Paquier wrote: > There is an argument to allow libpq to find out a service file for > a connection from the executable path. Note that oid2name can use a > connection string for connection, but not vacuumlo, so I somewhat > missed that. If you think that's not worth considering for now, I am fine to drop that part. What do you think about the updated patch attached then? -- Michael
Attachment
On 2019-09-05 01:59, Michael Paquier wrote: > On Wed, Sep 04, 2019 at 02:37:12PM +0200, Peter Eisentraut wrote: >>>> Do we need set_pglocale_pgservice() calls here if we're not doing NLS? >>>> Does the logging stuff require it? I'm not sure. >>> >>> The logging part does not require it, but this can be used for >>> PGSYSCONFDIR, no? >> >> How does PGSYSCONFDIR come into play here? > > There is an argument to allow libpq to find out a service file for > a connection from the executable path. Note that oid2name can use a > connection string for connection, but not vacuumlo, so I somewhat > missed that. Oh I see what's going on. PGSYSCONFDIR is used by libpq, and set_pglocale_pgservice() does some path mangling on PGSYSCONFDIR if set for Windows. Shouldn't libpq do that itself? Worth looking into but probably unrelated to your patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Patch looks good to me, please push. Generally speaking I find the 'progname' handling a bit silly (since we have it both as a variable in each program and also in logging.c separately), but that's not the fault of this patch, and this patch doesn't make it worse. That said, I think some other messages in vacuumlo can be made pg_log_somelevel(), based on occurrences of 'progname'. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Sep 05, 2019 at 09:59:51AM -0400, Alvaro Herrera wrote: > Patch looks good to me, please push. Thanks, applied without the calls to set_pglocale_pgservice(). > Generally speaking I find the 'progname' handling a bit silly (since we > have it both as a variable in each program and also in logging.c > separately), but that's not the fault of this patch, and this patch > doesn't make it worse. That said, I think some other messages in > vacuumlo can be made pg_log_somelevel(), based on occurrences of > 'progname'. That would mean moving some of the messages from stdout to stderr. We could do that. -- Michael