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.

diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index bdaa341be7b9110e2a42ececab20d0b6ccbb42b6..ab4b6ee55e3e6adee4e189d6873468db7bb22793 100644 (file)
--- 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 e901a2c92a1fc6918d421d707a5ff3543dd68409..29804aee8b801f8a0ab612548067b63384d5c8b5 100644 (file)
--- 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 d10a4d7f68a4f7d73058d92e666cd633ec3adcf0..6a45c5eb1cfd5203a36bcd175f6743fcc097dd9c 100644 (file)
--- 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);