On 2019-05-23 16:34, Tom Lane wrote:
> On the whole I think I could live with a policy that says "tuple counts
> shall be 'long long' when being passed around in code, but for persistent
> storage or wire-protocol transmission, use 'int64'".
>
> An alternative and much narrower policy is to say it's okay to do this
> with an int64 value:
>
> printf("processed %lld tuples", (long long) count);
>
> In such code, all we're assuming is long long >= 64 bits, which
> is completely safe per C99, and we dodge the need for a
> platform-varying format string.
Some combination of this seems quite reasonable.
Attached is a patch to implement this in a handful of cases that are
particularly verbose right now. I think those are easy wins.
(Also a second patch that makes use of %zu for size_t where this was not
yet done.)
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From b8cc16cd7172439104a59c2334347660dbc64929 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Thu, 6 Jun 2019 14:14:29 +0200
Subject: [PATCH 1/2] Unwind some workarounds for lack of portable int64 format
specifier
Because there is no portable int64/uint64 format specifier and we
can't stick macros like INT64_FORMAT into the middle of a translatable
string, we have been using various workarounds that put the number to
be printed into a string buffer first. Now that we always use our own
sprintf(), we can rely on %lld and %llu to work, so we can use those.
This patch undoes this workaround in a few places where it was
egregiously verbose.
Discussion:
https://www.postgresql.org/message-id/flat/CAH2-Wz%3DWbNxc5ob5NJ9yqo2RMJ0q4HXDS30GVCobeCvC9A1L9A%40mail.gmail.com
---
src/backend/access/transam/xlogreader.c | 16 +++-------------
src/backend/replication/basebackup.c | 11 +++--------
src/bin/pg_controldata/pg_controldata.c | 12 ++----------
src/bin/pg_resetwal/pg_resetwal.c | 13 ++-----------
src/bin/pg_rewind/libpq_fetch.c | 10 ++--------
src/bin/pg_test_timing/pg_test_timing.c | 12 +++---------
6 files changed, 15 insertions(+), 59 deletions(-)
diff --git a/src/backend/access/transam/xlogreader.c
b/src/backend/access/transam/xlogreader.c
index 88be7fe022..41dae916b4 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -783,20 +783,10 @@ XLogReaderValidatePageHeader(XLogReaderState *state,
XLogRecPtr recptr,
if (state->system_identifier &&
longhdr->xlp_sysid != state->system_identifier)
{
- char fhdrident_str[32];
- char sysident_str[32];
-
- /*
- * Format sysids separately to keep platform-dependent
format code
- * out of the translatable message string.
- */
- snprintf(fhdrident_str, sizeof(fhdrident_str),
UINT64_FORMAT,
- longhdr->xlp_sysid);
- snprintf(sysident_str, sizeof(sysident_str),
UINT64_FORMAT,
- state->system_identifier);
report_invalid_record(state,
- "WAL file is
from different database system: WAL file database system identifier is %s,
pg_control database system identifier is %s",
-
fhdrident_str, sysident_str);
+ "WAL file is
from different database system: WAL file database system identifier is %llu,
pg_control database system identifier is %llu",
+ (unsigned
long long) longhdr->xlp_sysid,
+ (unsigned
long long) state->system_identifier);
return false;
}
else if (longhdr->xlp_seg_size != state->wal_segment_size)
diff --git a/src/backend/replication/basebackup.c
b/src/backend/replication/basebackup.c
index c2978a949a..7a1b38466b 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -106,7 +106,7 @@ static TimestampTz throttled_last;
static XLogRecPtr startptr;
/* Total number of checksum failures during base backup. */
-static int64 total_checksum_failures;
+static long long int total_checksum_failures;
/* Do not verify checksums. */
static bool noverify_checksums = false;
@@ -607,14 +607,9 @@ perform_base_backup(basebackup_options *opt)
if (total_checksum_failures)
{
if (total_checksum_failures > 1)
- {
- char buf[64];
-
- snprintf(buf, sizeof(buf), INT64_FORMAT,
total_checksum_failures);
-
ereport(WARNING,
- (errmsg("%s total checksum verification
failures", buf)));
- }
+ (errmsg("%lld total checksum
verification failures", total_checksum_failures)));
+
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
errmsg("checksum verification failure during
base backup")));
diff --git a/src/bin/pg_controldata/pg_controldata.c
b/src/bin/pg_controldata/pg_controldata.c
index d955b97c0b..390ea0a939 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -99,7 +99,6 @@ main(int argc, char *argv[])
time_t time_tmp;
char pgctime_str[128];
char ckpttime_str[128];
- char sysident_str[32];
char mock_auth_nonce_str[MOCK_AUTH_NONCE_LEN * 2 + 1];
const char *strftime_fmt = "%c";
const char *progname;
@@ -222,13 +221,6 @@ main(int argc, char *argv[])
else
strcpy(xlogfilename, _("???"));
- /*
- * Format system_identifier and mock_authentication_nonce separately to
- * keep platform-dependent format code out of the translatable message
- * string.
- */
- snprintf(sysident_str, sizeof(sysident_str), UINT64_FORMAT,
- ControlFile->system_identifier);
for (i = 0; i < MOCK_AUTH_NONCE_LEN; i++)
snprintf(&mock_auth_nonce_str[i * 2], 3, "%02x",
(unsigned char)
ControlFile->mock_authentication_nonce[i]);
@@ -237,8 +229,8 @@ main(int argc, char *argv[])
ControlFile->pg_control_version);
printf(_("Catalog version number: %u\n"),
ControlFile->catalog_version_no);
- printf(_("Database system identifier: %s\n"),
- sysident_str);
+ printf(_("Database system identifier: %llu\n"),
+ (unsigned long long) ControlFile->system_identifier);
printf(_("Database cluster state: %s\n"),
dbState(ControlFile->state));
printf(_("pg_control last modified: %s\n"),
diff --git a/src/bin/pg_resetwal/pg_resetwal.c
b/src/bin/pg_resetwal/pg_resetwal.c
index 2734f87318..ff0f8ea5e7 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -748,26 +748,17 @@ GuessControlValues(void)
static void
PrintControlValues(bool guessed)
{
- char sysident_str[32];
-
if (guessed)
printf(_("Guessed pg_control values:\n\n"));
else
printf(_("Current pg_control values:\n\n"));
- /*
- * Format system_identifier separately to keep platform-dependent format
- * code out of the translatable message string.
- */
- snprintf(sysident_str, sizeof(sysident_str), UINT64_FORMAT,
- ControlFile.system_identifier);
-
printf(_("pg_control version number: %u\n"),
ControlFile.pg_control_version);
printf(_("Catalog version number: %u\n"),
ControlFile.catalog_version_no);
- printf(_("Database system identifier: %s\n"),
- sysident_str);
+ printf(_("Database system identifier: %llu\n"),
+ (unsigned long long) ControlFile.system_identifier);
printf(_("Latest checkpoint's TimeLineID: %u\n"),
ControlFile.checkPointCopy.ThisTimeLineID);
printf(_("Latest checkpoint's full_page_writes: %s\n"),
diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fetch.c
index d6cbe23926..37eccc3126 100644
--- a/src/bin/pg_rewind/libpq_fetch.c
+++ b/src/bin/pg_rewind/libpq_fetch.c
@@ -251,7 +251,6 @@ receiveFileChunks(const char *sql)
char *filename;
int filenamelen;
int64 chunkoff;
- char chunkoff_str[32];
int chunksize;
char *chunk;
@@ -327,13 +326,8 @@ receiveFileChunks(const char *sql)
continue;
}
- /*
- * Separate step to keep platform-dependent format code out of
- * translatable strings.
- */
- snprintf(chunkoff_str, sizeof(chunkoff_str), INT64_FORMAT,
chunkoff);
- pg_log_debug("received chunk for file \"%s\", offset %s, size
%d",
- filename, chunkoff_str, chunksize);
+ pg_log_debug("received chunk for file \"%s\", offset %lld, size
%d",
+ filename, (long long int) chunkoff,
chunksize);
open_target_file(filename, false);
diff --git a/src/bin/pg_test_timing/pg_test_timing.c
b/src/bin/pg_test_timing/pg_test_timing.c
index 6e2fd1ab8c..e14802372b 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -18,7 +18,7 @@ static uint64 test_timing(int32);
static void output(uint64 loop_count);
/* record duration in powers of 2 microseconds */
-int64 histogram[32];
+long long int histogram[32];
int
main(int argc, char *argv[])
@@ -190,14 +190,8 @@ output(uint64 loop_count)
Max(10, len3), header3);
for (i = 0; i <= max_bit; i++)
- {
- char buf[100];
-
- /* lame hack to work around INT64_FORMAT deficiencies */
- snprintf(buf, sizeof(buf), INT64_FORMAT, histogram[i]);
- printf("%*ld %*.5f %*s\n",
+ printf("%*ld %*.5f %*lld\n",
Max(6, len1), 1l << i,
Max(10, len2) - 1, (double) histogram[i] * 100 /
loop_count,
- Max(10, len3), buf);
- }
+ Max(10, len3), histogram[i]);
}
--
2.21.0
From 8b4ba199e2b61076f49df5220fa5c6310a82bc75 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Thu, 6 Jun 2019 14:32:54 +0200
Subject: [PATCH 2/2] Remove unnecessary casts from size_t to int
We can use the %zu format specifier directly, no need to cast to int.
---
src/interfaces/ecpg/preproc/ecpg.addons | 4 ++--
src/interfaces/ecpg/preproc/ecpg.trailer | 12 ++++++------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons
b/src/interfaces/ecpg/preproc/ecpg.addons
index 4e30375302..cbffd50e14 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -36,7 +36,7 @@ ECPG: stmtExecuteStmt block
/* It must be cut off double quotation because
new_variable() double-quotes. */
str[strlen(str) - 1] = '\0';
- sprintf(length, "%d", (int) strlen(str));
+ sprintf(length, "%zu", strlen(str));
add_variable_to_tail(&argsinsert,
new_variable(str, ECPGmake_simple_type(ECPGt_const, length, 0), 0),
&no_indicator);
}
output_statement(cat_str(3, mm_strdup("execute"),
mm_strdup("$0"), $1.type), 0, ECPGst_exec_with_exprlist);
@@ -63,7 +63,7 @@ ECPG: stmtPrepareStmt block
/* It must be cut off double quotation because
new_variable() double-quotes. */
str[strlen(str) - 1] = '\0';
- sprintf(length, "%d", (int) strlen(str));
+ sprintf(length, "%zu", strlen(str));
add_variable_to_tail(&argsinsert,
new_variable(str, ECPGmake_simple_type(ECPGt_const, length, 0), 0),
&no_indicator);
}
output_statement(cat_str(5, mm_strdup("prepare"),
mm_strdup("$0"), $1.type, mm_strdup("as"), $1.stmt), 0, ECPGst_prepare);
diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer
b/src/interfaces/ecpg/preproc/ecpg.trailer
index 1d58c778d9..b303a9cbd0 100644
--- a/src/interfaces/ecpg/preproc/ecpg.trailer
+++ b/src/interfaces/ecpg/preproc/ecpg.trailer
@@ -1099,7 +1099,7 @@ UsingValue: UsingConst
{
char *length = mm_alloc(32);
- sprintf(length, "%d", (int) strlen($1));
+ sprintf(length, "%zu", strlen($1));
add_variable_to_head(&argsinsert, new_variable($1,
ECPGmake_simple_type(ECPGt_const, length, 0), 0), &no_indicator);
}
| civar { $$ = EMPTY; }
@@ -1226,7 +1226,7 @@ IntConstVar: Iconst
{
char *length = mm_alloc(sizeof(int) * CHAR_BIT * 10 /
3);
- sprintf(length, "%d", (int) strlen($1));
+ sprintf(length, "%zu", strlen($1));
new_variable($1, ECPGmake_simple_type(ECPGt_const,
length, 0), 0);
$$ = $1;
}
@@ -1272,7 +1272,7 @@ AllConstVar: ecpg_fconst
{
char *length = mm_alloc(sizeof(int) * CHAR_BIT * 10 /
3);
- sprintf(length, "%d", (int) strlen($1));
+ sprintf(length, "%zu", strlen($1));
new_variable($1, ECPGmake_simple_type(ECPGt_const,
length, 0), 0);
$$ = $1;
}
@@ -1287,7 +1287,7 @@ AllConstVar: ecpg_fconst
char *length = mm_alloc(sizeof(int) * CHAR_BIT * 10 /
3);
char *var = cat2_str(mm_strdup("-"), $2);
- sprintf(length, "%d", (int) strlen(var));
+ sprintf(length, "%zu", strlen(var));
new_variable(var, ECPGmake_simple_type(ECPGt_const,
length, 0), 0);
$$ = var;
}
@@ -1297,7 +1297,7 @@ AllConstVar: ecpg_fconst
char *length = mm_alloc(sizeof(int) * CHAR_BIT * 10 /
3);
char *var = cat2_str(mm_strdup("-"), $2);
- sprintf(length, "%d", (int) strlen(var));
+ sprintf(length, "%zu", strlen(var));
new_variable(var, ECPGmake_simple_type(ECPGt_const,
length, 0), 0);
$$ = var;
}
@@ -1308,7 +1308,7 @@ AllConstVar: ecpg_fconst
char *var = $1 + 1;
var[strlen(var) - 1] = '\0';
- sprintf(length, "%d", (int) strlen(var));
+ sprintf(length, "%zu", strlen(var));
new_variable(var, ECPGmake_simple_type(ECPGt_const,
length, 0), 0);
$$ = var;
}
--
2.21.0
