Thread: [HACKERS] Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copyingfiles >2GB.

Robert Haas wrote:
> pg_rewind: Fix some problems when copying files >2GB.

I just noticed that this broke pg_rewind translation, because of the
INT64_FORMAT marker in the translatable string.  The message catalog now
has this:

msgid "received chunk for file \"%s\", offset "

for this source line:
       pg_log(PG_DEBUG, "received chunk for file \"%s\", offset " INT64_FORMAT ", size %d\n",              filename,
chunkoff,chunksize);
 

I don't think that this is terribly valuable as a translatable string,
so my preferred fix would be to have a new function pg_debug() here
where the messages are *not* marked for translations.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Aug 28, 2017 at 9:05 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> pg_rewind: Fix some problems when copying files >2GB.
>
> I just noticed that this broke pg_rewind translation, because of the
> INT64_FORMAT marker in the translatable string.  The message catalog now
> has this:
>
> msgid "received chunk for file \"%s\", offset "
>
> for this source line:
>
>         pg_log(PG_DEBUG, "received chunk for file \"%s\", offset " INT64_FORMAT ", size %d\n",
>                filename, chunkoff, chunksize);
>
> I don't think that this is terribly valuable as a translatable string,
> so my preferred fix would be to have a new function pg_debug() here
> where the messages are *not* marked for translations.

I am fine with however you want to handle it, but it seems odd to me
that we don't have a way of embedding INT64_FORMAT in a translatable
string.  Surely that's going to be a problem in some case, sometime,
isn't it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Mon, Aug 28, 2017 at 9:05 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Robert Haas wrote:
> >> pg_rewind: Fix some problems when copying files >2GB.
> >
> > I just noticed that this broke pg_rewind translation, because of the
> > INT64_FORMAT marker in the translatable string.  The message catalog now
> > has this:
> >
> > msgid "received chunk for file \"%s\", offset "
> >
> > for this source line:
> >
> >         pg_log(PG_DEBUG, "received chunk for file \"%s\", offset " INT64_FORMAT ", size %d\n",
> >                filename, chunkoff, chunksize);
> >
> > I don't think that this is terribly valuable as a translatable string,
> > so my preferred fix would be to have a new function pg_debug() here
> > where the messages are *not* marked for translations.
> 
> I am fine with however you want to handle it, but it seems odd to me
> that we don't have a way of embedding INT64_FORMAT in a translatable
> string.  Surely that's going to be a problem in some case, sometime,
> isn't it?

The way we do that elsewhere is to print out the value to a string
variable and then use %s in the translatable message.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Aug 28, 2017 at 10:16 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
>> I am fine with however you want to handle it, but it seems odd to me
>> that we don't have a way of embedding INT64_FORMAT in a translatable
>> string.  Surely that's going to be a problem in some case, sometime,
>> isn't it?
>
> The way we do that elsewhere is to print out the value to a string
> variable and then use %s in the translatable message.

Hmm.  OK.  That doesn't sound great, but if there's no better option...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Aug 28, 2017 at 11:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Aug 28, 2017 at 10:16 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>>> I am fine with however you want to handle it, but it seems odd to me
>>> that we don't have a way of embedding INT64_FORMAT in a translatable
>>> string.  Surely that's going to be a problem in some case, sometime,
>>> isn't it?
>>
>> The way we do that elsewhere is to print out the value to a string
>> variable and then use %s in the translatable message.
>
> Hmm.  OK.  That doesn't sound great, but if there's no better option...

I don't like breaking the abstraction of pg_log() with the existing
flags with some kind of pg_debug() layer. The set of APIs present now
in pg_rewind for logging is nice to have, and I think that those debug
messages should be translated. So what about the attached?
-- 
Michael

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

Michael Paquier <michael.paquier@gmail.com> writes:
> I don't like breaking the abstraction of pg_log() with the existing
> flags with some kind of pg_debug() layer. The set of APIs present now
> in pg_rewind for logging is nice to have, and I think that those debug
> messages should be translated. So what about the attached?

Your point about INT64_FORMAT not necessarily working with fprintf
is an outstanding reason not to keep it like it is.  I've not reviewed
this patch in detail but I think this is basically the way to fix it.
        regards, tom lane

On Tue, Aug 29, 2017 at 11:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> I don't like breaking the abstraction of pg_log() with the existing
>> flags with some kind of pg_debug() layer. The set of APIs present now
>> in pg_rewind for logging is nice to have, and I think that those debug
>> messages should be translated. So what about the attached?
>
> Your point about INT64_FORMAT not necessarily working with fprintf
> is an outstanding reason not to keep it like it is.  I've not reviewed
> this patch in detail but I think this is basically the way to fix it.

So, perhaps it would be better to fix that before the next point release?
-- 
Michael

Michael Paquier wrote:
> On Tue, Aug 29, 2017 at 11:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Michael Paquier <michael.paquier@gmail.com> writes:
> >> I don't like breaking the abstraction of pg_log() with the existing
> >> flags with some kind of pg_debug() layer. The set of APIs present now
> >> in pg_rewind for logging is nice to have, and I think that those debug
> >> messages should be translated. So what about the attached?
> >
> > Your point about INT64_FORMAT not necessarily working with fprintf
> > is an outstanding reason not to keep it like it is.  I've not reviewed
> > this patch in detail but I think this is basically the way to fix it.
> 
> So, perhaps it would be better to fix that before the next point release?

Sure, I'll get it done on Friday, or tomorrow if I can manage it.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Aug 31, 2017 at 8:39 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Michael Paquier wrote:
>> So, perhaps it would be better to fix that before the next point release?
>
> Sure, I'll get it done on Friday, or tomorrow if I can manage it.

Thanks, Álvaro.
--
Michael

Tom Lane wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
> > I don't like breaking the abstraction of pg_log() with the existing
> > flags with some kind of pg_debug() layer. The set of APIs present now
> > in pg_rewind for logging is nice to have, and I think that those debug
> > messages should be translated. So what about the attached?
> 
> Your point about INT64_FORMAT not necessarily working with fprintf
> is an outstanding reason not to keep it like it is.  I've not reviewed
> this patch in detail but I think this is basically the way to fix it.

Actually this code goes throgh vsnprintf, not fprintf, which should be
safe, so I removed that part of the comment, and pushed.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 9/4/17 06:03, Alvaro Herrera wrote:
> Tom Lane wrote:
>> Michael Paquier <michael.paquier@gmail.com> writes:
>>> I don't like breaking the abstraction of pg_log() with the existing
>>> flags with some kind of pg_debug() layer. The set of APIs present now
>>> in pg_rewind for logging is nice to have, and I think that those debug
>>> messages should be translated. So what about the attached?
>>
>> Your point about INT64_FORMAT not necessarily working with fprintf
>> is an outstanding reason not to keep it like it is.  I've not reviewed
>> this patch in detail but I think this is basically the way to fix it.
> 
> Actually this code goes throgh vsnprintf, not fprintf, which should be
> safe, so I removed that part of the comment, and pushed.

Is there a reason this was not backpatched to 9.5?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Sep 7, 2017 at 10:11 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 9/4/17 06:03, Alvaro Herrera wrote:
>> Tom Lane wrote:
>>> Michael Paquier <michael.paquier@gmail.com> writes:
>>>> I don't like breaking the abstraction of pg_log() with the existing
>>>> flags with some kind of pg_debug() layer. The set of APIs present now
>>>> in pg_rewind for logging is nice to have, and I think that those debug
>>>> messages should be translated. So what about the attached?
>>>
>>> Your point about INT64_FORMAT not necessarily working with fprintf
>>> is an outstanding reason not to keep it like it is.  I've not reviewed
>>> this patch in detail but I think this is basically the way to fix it.
>>
>> Actually this code goes throgh vsnprintf, not fprintf, which should be
>> safe, so I removed that part of the comment, and pushed.
>
> Is there a reason this was not backpatched to 9.5?

Indeed. Please note that cherry-picking the fix from 23c1f0a works just fine.
-- 
Michael

Peter Eisentraut wrote:
> On 9/4/17 06:03, Alvaro Herrera wrote:
> > Tom Lane wrote:
> >> Michael Paquier <michael.paquier@gmail.com> writes:
> >>> I don't like breaking the abstraction of pg_log() with the existing
> >>> flags with some kind of pg_debug() layer. The set of APIs present now
> >>> in pg_rewind for logging is nice to have, and I think that those debug
> >>> messages should be translated. So what about the attached?
> >>
> >> Your point about INT64_FORMAT not necessarily working with fprintf
> >> is an outstanding reason not to keep it like it is.  I've not reviewed
> >> this patch in detail but I think this is basically the way to fix it.
> > 
> > Actually this code goes throgh vsnprintf, not fprintf, which should be
> > safe, so I removed that part of the comment, and pushed.
> 
> Is there a reason this was not backpatched to 9.5?

No, I'll backpatch later.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut wrote:
> On 9/4/17 06:03, Alvaro Herrera wrote:
> > Tom Lane wrote:
> >> Michael Paquier <michael.paquier@gmail.com> writes:
> >>> I don't like breaking the abstraction of pg_log() with the existing
> >>> flags with some kind of pg_debug() layer. The set of APIs present now
> >>> in pg_rewind for logging is nice to have, and I think that those debug
> >>> messages should be translated. So what about the attached?
> >>
> >> Your point about INT64_FORMAT not necessarily working with fprintf
> >> is an outstanding reason not to keep it like it is.  I've not reviewed
> >> this patch in detail but I think this is basically the way to fix it.
> > 
> > Actually this code goes throgh vsnprintf, not fprintf, which should be
> > safe, so I removed that part of the comment, and pushed.
> 
> Is there a reason this was not backpatched to 9.5?

Done.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services