On Thu, 8 Jul 2021 at 05:30, David Rowley <dgrowle...@gmail.com> wrote:
>
> On Thu, 8 Jul 2021 at 13:31, David Rowley <dgrowle...@gmail.com> wrote:
> > It feels like if we're going to fix this negative rounding thing then
> > we should maybe do it and backpatch a fix then rebase this work on top
> > of that.
Yes, that was my thinking too.
> Here's a patch which I believe makes pg_size_pretty() and
> pg_size_pretty_numeric() match in regards to negative values.
LGTM, except I think it's worth also making the numeric code not refer
to bit shifting either.
> Maybe this plus your regression test would be ok to back-patch?
+1
Here's an update with matching updates to the numeric code, plus the
regression tests.
Regards,
Dean
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
new file mode 100644
index 9c39e7d..d03e103
--- 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));
}
@@ -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));
}
diff --git a/src/test/regress/expected/dbsize.out b/src/test/regress/expected/dbsize.out
new file mode 100644
index e901a2c..29804ae
--- 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_si
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
new file mode 100644
index d10a4d7..6a45c5e
--- 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_si
(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);
