Skip to content

P
Postgres FD Implementation
Commit 6de3a21b authored by David Rowley's avatar David Rowley
Browse files
Options

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 8a1fab36. 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.
parent 9893d512
Hide whitespace changes
Inline Side-by-side
Showing with 79 additions and 16 deletions
src/backend/utils/adt/dbsize.c
View file @ 6de3a21b
......@@ -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));
}
......@@ -621,13 +625,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_numeric;
Datum result;
divisor_numeric = NumericGetDatum(int64_to_numeric(((int64) 1) << count));
divisor_numeric = NumericGetDatum(int64_to_numeric(divisor));
result = DirectFunctionCall2(numeric_div_trunc, d, divisor_numeric);
return DatumGetNumeric(result);
}
......@@ -650,8 +654,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))
{
......@@ -660,8 +664,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);
......@@ -669,8 +674,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))
{
......@@ -679,8 +684,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));
}
......
src/test/regress/expected/dbsize.out
View file @ 6de3a21b
......@@ -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);
......
src/test/regress/sql/dbsize.sql
View file @ 6de3a21b
......@@ -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);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment