Horton

Thread

Commits

  1. Fix translatable string

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

  1. pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Robert Haas <rhaas@postgresql.org> — 2017-07-22T02:10:27Z

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

When incrementally updating a file larger than 2GB, the old code could
either fail outright (if the client asked the server for bytes beyond
the 2GB boundary) or fail to copy all the blocks that had actually
been modified (if the server reported a file size to the client in
excess of 2GB), resulting in data corruption.  Generally, such files
won't occur anyway, but they might if using a non-default segment size
or if there the directory contains stray files unrelated to
PostgreSQL.  Fix by a more prudent choice of data types.

Even with these improvements, this code still uses a mix of different
types (off_t, size_t, uint64, int64) to represent file sizes and
offsets, not all of which necessarily have the same width or
signedness, so further cleanup might be in order here.  However, at
least now they all have the potential to be 64 bits wide on 64-bit
platforms.

Kuntal Ghosh and Michael Paquier, with a tweak by me.

Discussion: http://postgr.es/m/CAGz5QC+8gbkz=Brp0TgoKNqHWTzonbPtPex80U0O6Uh_bevbaA@mail.gmail.com

Branch
------
REL9_6_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/73fbf3d3d0a59d79f75a8202e03863fe462d052f

Modified Files
--------------
src/bin/pg_rewind/libpq_fetch.c | 47 ++++++++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 12 deletions(-)

  2. Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-08-28T13:05:45Z

    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

  3. Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Robert Haas <robertmhaas@gmail.com> — 2017-08-28T14:08:44Z

    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

  4. Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-08-28T14:16:31Z

    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

  5. Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Robert Haas <robertmhaas@gmail.com> — 2017-08-28T14:24:14Z

    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

  6. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Michael Paquier <michael.paquier@gmail.com> — 2017-08-29T02:22:12Z

    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

  7. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-29T02:26:24Z

    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

  8. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Michael Paquier <michael.paquier@gmail.com> — 2017-08-30T22:16:59Z

    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

  9. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-08-30T23:39:18Z

    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

  10. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Michael Paquier <michael.paquier@gmail.com> — 2017-08-30T23:50:54Z

    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

  11. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-09-04T10:03:57Z

    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

  12. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-07T01:11:51Z

    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

  13. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Michael Paquier <michael.paquier@gmail.com> — 2017-09-07T01:28:57Z

    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

  14. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-09-07T07:22:17Z

    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

  15. Re: Re: [COMMITTERS] pgsql: pg_rewind: Fix some problems when copying files >2GB.

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-09-07T09:42:26Z

    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