From 6f88b68ff4adfe17a80c0befdf9bb8b0a8218def Mon Sep 17 00:00:00 2001 From: David Rowley Date: Fri, 9 Jul 2021 14:04:49 +1200 Subject: [PATCH] Fix incorrect return value in pg_size_pretty(bigint) Due to how pg_size_pretty(bigint) was implemented, it's possible that when given a negative number of bytes that the returning value would not match the equivalent positive return value when given the equivalent positive number of bytes. This was due to two separate issues. 1. The function used bit shifting to convert the number of bytes into larger units. The rounding performed by bit shifting is not the same as dividing. For example -3 >> 1 = -2, but -3 / 2 = -1. These two operations are only equivalent with positive numbers. 2. The half_rounded() macro rounded towards positive infinity. This meant that negative numbers rounded towards zero and positive numbers rounded away from zero. Here we fix #1 by dividing the values instead of bit shifting. We fix #2 by adjusting the half_rounded macro always to round away from zero. Additionally, adjust the pg_size_pretty(numeric) function to be more explicit that it's using division rather than bit shifting. A casual observer might have believed bit shifting was used due to a static function being named numeric_shift_right. However, that function was calculating the divisor from the number of bits and performed division. Here we make that more clear. This change is just cosmetic and does not affect the return value of the numeric version of the function. Here we also add a set of regression tests both versions of pg_size_pretty() which test the values directly before and after the function switches to the next unit. This bug was introduced in 8a1fab36a. Prior to that negative values were always displayed in bytes. Author: Dean Rasheed, David Rowley Discussion: https://postgr.es/m/CAEZATCXnNW4HsmZnxhfezR5FuiGgp+mkY4AzcL5eRGO4fuadWg@mail.gmail.com Backpatch-through: 9.6, where the bug was introduced. --- src/backend/utils/adt/dbsize.c | 39 ++++++++++++++------------ src/test/regress/expected/dbsize.out | 42 ++++++++++++++++++++++++++++ src/test/regress/sql/dbsize.sql | 16 +++++++++++ 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c index 70d7f86c8d..89253f651a 100644 --- a/src/backend/utils/adt/dbsize.c +++ b/src/backend/utils/adt/dbsize.c @@ -31,8 +31,8 @@ #include "utils/relmapper.h" #include "utils/syscache.h" -/* Divide by two and round towards positive infinity. */ -#define half_rounded(x) (((x) + ((x) < 0 ? 0 : 1)) / 2) +/* Divide by two and round away from zero */ +#define half_rounded(x) (((x) + ((x) < 0 ? -1 : 1)) / 2) /* Return physical size of directory contents, or 0 if dir doesn't exist */ static int64 @@ -542,25 +542,29 @@ pg_size_pretty(PG_FUNCTION_ARGS) snprintf(buf, sizeof(buf), INT64_FORMAT " bytes", size); else { - size >>= 9; /* keep one extra bit for rounding */ + /* + * We use divide instead of bit shifting so that behavior matches for + * both positive and negative size values. + */ + size /= (1 << 9); /* keep one extra bit for rounding */ if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " kB", half_rounded(size)); else { - size >>= 10; + size /= (1 << 10); if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " MB", half_rounded(size)); else { - size >>= 10; + size /= (1 << 10); if (Abs(size) < limit2) snprintf(buf, sizeof(buf), INT64_FORMAT " GB", half_rounded(size)); else { - size >>= 10; + size /= (1 << 10); snprintf(buf, sizeof(buf), INT64_FORMAT " TB", half_rounded(size)); } @@ -629,15 +633,13 @@ numeric_half_rounded(Numeric n) } static Numeric -numeric_shift_right(Numeric n, unsigned count) +numeric_truncated_divide(Numeric n, int64 divisor) { Datum d = NumericGetDatum(n); - Datum divisor_int64; Datum divisor_numeric; Datum result; - divisor_int64 = Int64GetDatum((int64) (1 << count)); - divisor_numeric = DirectFunctionCall1(int8_numeric, divisor_int64); + divisor_numeric = DirectFunctionCall1(int8_numeric, divisor); result = DirectFunctionCall2(numeric_div_trunc, d, divisor_numeric); return DatumGetNumeric(result); } @@ -660,8 +662,8 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS) else { /* keep one extra bit for rounding */ - /* size >>= 9 */ - size = numeric_shift_right(size, 9); + /* size /= (1 << 9) */ + size = numeric_truncated_divide(size, 1 << 9); if (numeric_is_less(numeric_absolute(size), limit2)) { @@ -670,8 +672,9 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS) } else { - /* size >>= 10 */ - size = numeric_shift_right(size, 10); + /* size /= (1 << 10) */ + size = numeric_truncated_divide(size, 1 << 10); + if (numeric_is_less(numeric_absolute(size), limit2)) { size = numeric_half_rounded(size); @@ -679,8 +682,8 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS) } else { - /* size >>= 10 */ - size = numeric_shift_right(size, 10); + /* size /= (1 << 10) */ + size = numeric_truncated_divide(size, 1 << 10); if (numeric_is_less(numeric_absolute(size), limit2)) { @@ -689,8 +692,8 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS) } else { - /* size >>= 10 */ - size = numeric_shift_right(size, 10); + /* size /= (1 << 10) */ + size = numeric_truncated_divide(size, 1 << 10); size = numeric_half_rounded(size); result = psprintf("%s TB", numeric_to_cstring(size)); } diff --git a/src/test/regress/expected/dbsize.out b/src/test/regress/expected/dbsize.out index e901a2c92a..29804aee8b 100644 --- a/src/test/regress/expected/dbsize.out +++ b/src/test/regress/expected/dbsize.out @@ -35,6 +35,48 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM 1000000000000000.5 | 909 TB | -909 TB (12 rows) +-- test where units change up +SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM + (VALUES (10239::bigint), (10240::bigint), + (10485247::bigint), (10485248::bigint), + (10736893951::bigint), (10736893952::bigint), + (10994579406847::bigint), (10994579406848::bigint), + (11258449312612351::bigint), (11258449312612352::bigint)) x(size); + size | pg_size_pretty | pg_size_pretty +-------------------+----------------+---------------- + 10239 | 10239 bytes | -10239 bytes + 10240 | 10 kB | -10 kB + 10485247 | 10239 kB | -10239 kB + 10485248 | 10 MB | -10 MB + 10736893951 | 10239 MB | -10239 MB + 10736893952 | 10 GB | -10 GB + 10994579406847 | 10239 GB | -10239 GB + 10994579406848 | 10 TB | -10 TB + 11258449312612351 | 10239 TB | -10239 TB + 11258449312612352 | 10240 TB | -10240 TB +(10 rows) + +SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM + (VALUES (10239::numeric), (10240::numeric), + (10485247::numeric), (10485248::numeric), + (10736893951::numeric), (10736893952::numeric), + (10994579406847::numeric), (10994579406848::numeric), + (11258449312612351::numeric), (11258449312612352::numeric)) x(size); + size | pg_size_pretty | pg_size_pretty +-------------------+----------------+---------------- + 10239 | 10239 bytes | -10239 bytes + 10240 | 10 kB | -10 kB + 10485247 | 10239 kB | -10239 kB + 10485248 | 10 MB | -10 MB + 10736893951 | 10239 MB | -10239 MB + 10736893952 | 10 GB | -10 GB + 10994579406847 | 10239 GB | -10239 GB + 10994579406848 | 10 TB | -10 TB + 11258449312612351 | 10239 TB | -10239 TB + 11258449312612352 | 10240 TB | -10240 TB +(10 rows) + +-- pg_size_bytes() tests SELECT size, pg_size_bytes(size) FROM (VALUES ('1'), ('123bytes'), ('1kB'), ('1MB'), (' 1 GB'), ('1.5 GB '), ('1TB'), ('3000 TB'), ('1e6 MB')) x(size); diff --git a/src/test/regress/sql/dbsize.sql b/src/test/regress/sql/dbsize.sql index d10a4d7f68..6a45c5eb1c 100644 --- a/src/test/regress/sql/dbsize.sql +++ b/src/test/regress/sql/dbsize.sql @@ -11,6 +11,22 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (1000000000.5::numeric), (1000000000000.5::numeric), (1000000000000000.5::numeric)) x(size); +-- test where units change up +SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM + (VALUES (10239::bigint), (10240::bigint), + (10485247::bigint), (10485248::bigint), + (10736893951::bigint), (10736893952::bigint), + (10994579406847::bigint), (10994579406848::bigint), + (11258449312612351::bigint), (11258449312612352::bigint)) x(size); + +SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM + (VALUES (10239::numeric), (10240::numeric), + (10485247::numeric), (10485248::numeric), + (10736893951::numeric), (10736893952::numeric), + (10994579406847::numeric), (10994579406848::numeric), + (11258449312612351::numeric), (11258449312612352::numeric)) x(size); + +-- pg_size_bytes() tests SELECT size, pg_size_bytes(size) FROM (VALUES ('1'), ('123bytes'), ('1kB'), ('1MB'), (' 1 GB'), ('1.5 GB '), ('1TB'), ('3000 TB'), ('1e6 MB')) x(size);