8000 Fix incorrect return value in pg_size_pretty(bigint) · postgrespro/postgres@45bad6a · GitHub
[go: up one dir, main page]
Skip to content

    • Commit 45bad6a

    Browse files
    david-rowleydavid-rowley
    committed
    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 8a1fab3. 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.
    1 parent a9460db commit 45bad6a

    File tree

    3 files changed

    +79
    -18
    lines changed

    3 files changed

    +79
    -18
    lines changed

    ‎src/backend/utils/adt/dbsize.c

    Lines changed: 21 additions & 18 deletions
    Original file line numberDiff line numberDiff line change
    @@ -31,8 +31,8 @@
    3131
    #include "utils/relmapper.h"
    3232
    #include "utils/syscache.h"
    3333

    34-
    /* Divide by two and round towards positive infinity. */
    35-
    #define half_rounded(x) (((x) + ((x) < 0 ? 0 : 1)) / 2)
    34+
    /* Divide by two and round away from zero */
    35+
    #define half_rounded(x) (((x) + ((x) < 0 ? -1 : 1)) / 2)
    3636

    3737
    /* Return physical size of directory contents, or 0 if dir doesn't exist */
    3838
    static int64
    @@ -542,25 +542,29 @@ pg_size_pretty(PG_FUNCTION_ARGS)
    542542
    snprintf(buf, sizeof(buf), INT64_FORMAT " bytes", size);
    543543
    else
    544544
    {
    545-
    size >>= 9; /* keep one extra bit for rounding */
    545+
    /*
    546+
    * We use divide instead of bit shifting so that behavior matches for
    547+
    * both positive and negative size values.
    548+
    */
    549+
    size /= (1 << 9); /* keep one extra bit for rounding */
    546550
    if (Abs(size) < limit2)
    547551
    snprintf(buf, sizeof(buf), INT64_FORMAT " kB",
    548552
    half_rounded(size));
    549553
    else
    550554
    {
    551-
    size >>= 10;
    555+
    size /= (1 << 10);
    552556
    if (Abs(size) < limit2)
    553557
    snprintf(buf, sizeof(buf), INT64_FORMAT " MB",
    554558
    half_rounded(size));
    555559
    else
    556560
    {
    557-
    size >>= 10;
    561+
    size /= (1 << 10);
    558562
    if (Abs(size) < limit2)
    559563
    snprintf(buf, sizeof(buf), INT64_FORMAT " GB",
    560564
    half_rounded(size));
    561565
    else
    562566
    {
    563-
    size >>= 10;
    567+
    size /= (1 << 10);
    564568
    snprintf(buf, sizeof(buf), INT64_FORMAT " TB",
    565569
    half_rounded(size));
    566570
    10000 }
    @@ -629,15 +633,13 @@ numeric_half_rounded(Numeric n)
    629633
    }
    630634

    631635
    static Numeric
    632-
    numeric_shift_right(Numeric n, unsigned count)
    636+
    numeric_truncated_divide(Numeric n, int64 divisor)
    633637
    {
    634638
    Datum d = NumericGetDatum(n);
    635-
    Datum divisor_int64;
    636639
    Datum divisor_numeric;
    637640
    Datum result;
    638641

    639-
    divisor_int64 = Int64GetDatum((int64) (1 << count));
    640-
    divisor_numeric = DirectFunctionCall1(int8_numeric, divisor_int64);
    642+
    divisor_numeric = DirectFunctionCall1(int8_numeric, divisor);
    641643
    result = DirectFunctionCall2(numeric_div_trunc, d, divisor_numeric);
    642644
    return DatumGetNumeric(result);
    643645
    }
    @@ -660,8 +662,8 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
    660662
    else
    661663
    {
    662664
    /* keep one extra bit for rounding */
    663-
    /* size >>= 9 */
    664-
    size = numeric_shift_right(size, 9);
    665+
    /* size /= (1 << 9) */
    666+
    size = numeric_truncated_divide(size, 1 << 9);
    665667

    666668
    if (numeric_is_less(numeric_absolute(size), limit2))
    667669
    {
    @@ -670,17 +672,18 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
    670672
    }
    671673
    else
    672674
    {
    673-
    /* size >>= 10 */
    674-
    size = numeric_shift_right(size, 10);
    675+
    /* size /= (1 << 10) */
    676+
    size = numeric_truncated_divide(size, 1 << 10);
    677+
    675678
    if (numeric_is_less(numeric_absolute(size), limit2))
    676679
    {
    677680
    size = numeric_half_rounded(size);
    678681
    result = psprintf("%s MB", numeric_to_cstring(size));
    679682
    }
    680683
    else
    681684
    {
    682-
    /* size >>= 10 */
    683-
    size = numeric_shift_right(size, 10);
    685+
    /* size /= (1 << 10) */
    686+
    size = numeric_truncated_divide(size, 1 << 10);
    684687

    685688
    if (numeric_is_less(numeric_absolute(size), limit2))
    686689
    {
    @@ -689,8 +692,8 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)
    689692
    }
    690693
    else
    691694
    {
    692-
    /* size >>= 10 */
    693-
    size = numeric_shift_right(size, 10);
    695+
    /* size /= (1 << 10) */
    696+
    size = numeric_truncated_divide(size, 1 << 10);
    694697
    size = numeric_half_rounded(size);
    695698
    result = psprintf("%s TB", numeric_to_cstring(size));
    696699
    }

    ‎src/test/regress/expected/dbsize.out

    Lines changed: 42 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -35,6 +35,48 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
    3535
    1000000000000000.5 | 909 TB | -909 TB
    3636
    (12 rows)
    3737

    38+
    -- test where units change up
    39+
    SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
    40+
    (VALUES (10239::bigint), (10240::bigint),
    41+
    (10485247::bigint), (10485248::bigint),
    42+
    (10736893951::bigint), (10736893952::bigint),
    43+
    (10994579406847::bigint), (10994579406848::bigint),
    44+
    (11258449312612351::bigint), (11258449312612352::bigint)) x(size);
    45+
    size | pg_size_pretty | pg_size_pretty
    46+
    -------------------+----------------+----------------
    47+
    10239 | 10239 bytes | -10239 bytes
    48+
    10240 | 10 kB | -10 kB
    49+
    10485247 | 10239 kB | -10239 kB
    50+
    10485248 | 10 MB | -10 MB
    51+
    10736893951 | 10239 MB | -10239 MB
    52+
    10736893952 | 10 GB | -10 GB
    53+
    10994579406847 | 10239 GB | -10239 GB
    54+
    10994579406848 | 10 TB | -10 TB
    55+
    11258449312612351 | 10239 TB | -10239 TB
    56+
    11258449312612352 | 10240 TB | -10240 TB
    57+
    (10 rows)
    58+
    59+
    SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
    60+
    (VALUES (10239::numeric), (10240::numeric),
    61+
    (10485247::numeric), (10485248::numeric),
    62+
    (10736893951::numeric), (10736893952::numeric),
    63+
    (10994579406847::numeric), (10994579406848::numeric),
    64+
    (11258449312612351::numeric), (11258449312612352::numeric)) x(size);
    65+
    size | pg_size_pretty | pg_size_pretty
    66+
    -------------------+----------------+----------------
    67+
    10239 | 10239 bytes | -10239 bytes
    68+
    10240 | 10 kB | -10 kB
    69+
    10485247 | 10239 kB | -10239 kB
    70+
    10485248 | 10 MB | -10 MB
    71+
    10736893951 | 10239 MB | -10239 MB
    72+
    10736893952 | 10 GB | -10 GB
    73+
    10994579406847 | 10239 GB | -10239 GB
    74+
    10994579406848 | 10 TB | -10 TB
    75+
    11258449312612351 | 10239 TB | -10239 TB
    76+
    11258449312612352 | 10240 TB | -10240 TB
    77+
    (10 rows)
    78+
    79+
    -- pg_size_bytes() tests
    3880
    SELECT size, pg_size_bytes(size) FROM
    3981
    (VALUES ('1'), ('123bytes'), ('1kB'), ('1MB'), (' 1 GB'), ('1.5 GB '),
    4082
    ('1TB'), ('3000 TB'), ('1e6 MB')) x(size);

    ‎src/test/regress/sql/dbsize.sql

    Lines changed: 16 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -11,6 +11,22 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
    1111
    (1000000000.5::numeric), (1000000000000.5::numeric),
    1212
    (1000000000000000.5::numeric)) x(size);
    1313

    14+
    -- test where units change up
    15+
    SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
    16+
    (VALUES (10239::bigint), (10240::bigint),
    17+
    (10485247::bigint), (10485248::bigint),
    18+
    (10736893951::bigint), (10736893952::bigint),
    19+
    (10994579406847::bigint), (10994579406848::bigint),
    20+
    (11258449312612351::bigint), (11258449312612352::bigint)) x(size);
    21+
    22+
    SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
    23+
    (VALUES (10239::numeric), (10240::numeric),
    24+
    (10485247::numeric), (10485248::numeric),
    25+
    (10736893951::numeric), (10736893952::numeric),
    26+
    (10994579406847::numeric), (10994579406848::numeric),
    27+
    (11258449312612351::numeric), (11258449312612352::numeric)) x(size);
    28+
    29+
    -- pg_size_bytes() tests
    1430
    SELECT size, pg_size_bytes(size) FROM
    1531
    (VALUES ('1'), ('123bytes'), ('1kB'), ('1MB'), (' 1 GB'), ('1.5 GB '),
    1632
    ('1TB'), ('3000 TB'), ('1e6 MB')) x(size);

    0 commit comments

    Comments
     (0)
    0