Message-ID: <552E1985.1060202@iki.fi>
Date: Wed, 15 Apr 2015 10:55:49 +0300
From: Heikki Linnakangas <hlinnaka@iki.fi>
Reply-To: hlinnaka@iki.fi
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:31.0) Gecko/20100101 Icedove/31.6.0
MIME-Version: 1.0
To: Fujii Masao <masao.fujii@gmail.com>,
 Alvaro Herrera <alvherre@2ndquadrant.com>
CC: Michael Paquier <michael.paquier@gmail.com>,
 Peter Eisentraut <peter_e@gmx.net>, Fujii Masao <fujii@postgresql.org>,
 pgsql-committers <pgsql-committers@postgresql.org>
Subject: Re: pgsql: Mark the second argument of pg_log as the
 translatable string in
References: <E1YffOB-0006ry-RI@gemulon.postgresql.org>
	<5529C5A7.3070708@gmx.net>	<20150412011715.GK4369@alvh.no-ip.org>
	<CAB7nPqQP8NU3WztTG8Snd4k2S5dLx1CN0dRCzcuFF1GuyqJATQ@mail.gmail.com>
	<20150413171705.GQ4369@alvh.no-ip.org>
 <CAHGQGwEgBEyXXrV6G=9ZhBOxypo06JrGQGBgwv+T-+PcjaWvfA@mail.gmail.com>
In-Reply-To: 
 <CAHGQGwEgBEyXXrV6G=9ZhBOxypo06JrGQGBgwv+T-+PcjaWvfA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-committers-owner@postgresql.org

On 04/15/2015 06:41 AM, Fujii Masao wrote:
> On Tue, Apr 14, 2015 at 2:17 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Michael Paquier wrote:
>>> On Sun, Apr 12, 2015 at 10:17 AM, Alvaro Herrera wrote:
>>>> What pg_basebackup's progress_report() does is have the message in the
>>>> translatable part not include the \r; the \r is in a separate fprintf()
>>>> call.
>>>
>>> Like the attached then.
>>
>> Not a fan of this approach, because now this function knows that
>> pg_log(PG_PROGRESS) is equivalent to printf().  This abstraction is a
>> bit leaky, isn't it ...  Probably not worth sweating about, though.
>>
>>> diff --git a/src/bin/pg_rewind/logging.c b/src/bin/pg_rewind/logging.c
>>> index aba12d8..3e2dc76 100644
>>> --- a/src/bin/pg_rewind/logging.c
>>> +++ b/src/bin/pg_rewind/logging.c
>>> @@ -134,7 +134,8 @@ progress_report(bool force)
>>>        snprintf(fetch_size_str, sizeof(fetch_size_str), INT64_FORMAT,
>>>                         fetch_size / 1024);
>>>
>>> -     pg_log(PG_PROGRESS, "%*s/%s kB (%d%%) copied\r",
>>> +     pg_log(PG_PROGRESS, "%*s/%s kB (%d%%) copied",
>>>                   (int) strlen(fetch_size_str), fetch_done_str, fetch_size_str,
>>>                   percent);
>>> +     printf("\r");
>>>   }
>
> So could you elaborate your "favorite" approach?
>
> Now pg_log() calls printf() and fflush(stdout). So '\r' is printed after fflush.
> It's a bit strange. Maybe we can just replace pg_log() with printf() here.

A better solution from a modularity point of view would be to add a new 
function, pg_progress() for this. It would print the line with pg_log() 
and then print the \r to the end. Then the caller wouldn't need to know 
whether the progress messages are going to stdout, stderr, or somewhere 
else entirely.

> Another question is; should we output the progress report to stderr rather
> than stdout? I thought this because I found that pg_basebackup reports
> the progress to stderr.

Yeah, probably. We should go through all the output and figure out where 
each kind of message needs to do. Should follow the principle Alvaro 
laid out 
(http://www.postgresql.org/message-id/20150407205320.GN4369@alvh.no-ip.org), 
and also make sure it's consistent with pg_basebackup and other tools. 
Michael's patch changed some of the logging but we should take a more 
holistic look at the situation. And add a comment somewhere explaining 
the principle.

- Heikki


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers