pgsql: pg_rewind: Fix some problems when copying files >2GB.
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: /messages/by-id/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(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
rewind-int64-log.patchapplication/octet-stream; name=rewind-int64-log.patchDownload
diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c
index a6ff4e3817..ee62c6db88 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -272,6 +272,7 @@ receiveFileChunks(const char *sql)
int64 chunkoff;
int chunksize;
char *chunk;
+ char chunkoff_str[32];
switch (PQresultStatus(res))
{
@@ -342,8 +343,14 @@ receiveFileChunks(const char *sql)
continue;
}
- pg_log(PG_DEBUG, "received chunk for file \"%s\", offset " INT64_FORMAT ", size %d\n",
- filename, chunkoff, chunksize);
+ /*
+ * Separate step to keep platform-dependent format code out of
+ * translatable strings. And we only test for INT64_FORMAT
+ * availability in snprintf, not fprintf.
+ */
+ snprintf(chunkoff_str, sizeof(chunkoff_str), INT64_FORMAT, chunkoff);
+ pg_log(PG_DEBUG, "received chunk for file \"%s\", offset %s, size %d\n",
+ filename, chunkoff_str, chunksize);
open_target_file(filename, false);
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers