Обсуждение: 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

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

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

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

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