Fix incorrect return value in pg_size_pretty(bigint)

david-rowley · david-rowley · commit efc42a1e18b5 · 2021-07-09T14:04:56.000+12:00
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.
diff --git 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,17 +672,18 @@ 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);
 				result = psprintf("%s MB", numeric_to_cstring(size));
 			}
 			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
@@ -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
@@ -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);

Original file line number	Diff line number	Diff line change
`@@ -31,8 +31,8 @@`
`31`	`31`	`#include "utils/relmapper.h"`
`32`	`32`	`#include "utils/syscache.h"`
`33`	`33`
`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)`
`36`	`36`
`37`	`37`	`/* Return physical size of directory contents, or 0 if dir doesn't exist */`
`38`	`38`	`static int64`
`@@ -542,25 +542,29 @@ pg_size_pretty(PG_FUNCTION_ARGS)`
`542`	`542`	`snprintf(buf, sizeof(buf), INT64_FORMAT " bytes", size);`
`543`	`543`	`else`
`544`	`544`	`{`
`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 */`
`546`	`550`	`if (Abs(size) < limit2)`
`547`	`551`	`snprintf(buf, sizeof(buf), INT64_FORMAT " kB",`
`548`	`552`	`half_rounded(size));`
`549`	`553`	`else`
`550`	`554`	`{`
`551`		`- size >>= 10;`
	`555`	`+ size /= (1 << 10);`
`552`	`556`	`if (Abs(size) < limit2)`
`553`	`557`	`snprintf(buf, sizeof(buf), INT64_FORMAT " MB",`
`554`	`558`	`half_rounded(size));`
`555`	`559`	`else`
`556`	`560`	`{`
`557`		`- size >>= 10;`
	`561`	`+ size /= (1 << 10);`
`558`	`562`	`if (Abs(size) < limit2)`
`559`	`563`	`snprintf(buf, sizeof(buf), INT64_FORMAT " GB",`
`560`	`564`	`half_rounded(size));`
`561`	`565`	`else`
`562`	`566`	`{`
`563`		`- size >>= 10;`
	`567`	`+ size /= (1 << 10);`
`564`	`568`	`snprintf(buf, sizeof(buf), INT64_FORMAT " TB",`
`565`	`569`	`half_rounded(size));`
`566`	`570`	`}`
`@@ -629,15 +633,13 @@ numeric_half_rounded(Numeric n)`
`629`	`633`	`}`
`630`	`634`
`631`	`635`	`static Numeric`
`632`		`-numeric_shift_right(Numeric n, unsigned count)`
	`636`	`+numeric_truncated_divide(Numeric n, int64 divisor)`
`633`	`637`	`{`
`634`	`638`	`Datum d = NumericGetDatum(n);`
`635`		`- Datum divisor_int64;`
`636`	`639`	`Datum divisor_numeric;`
`637`	`640`	`Datum result;`
`638`	`641`
`639`		`- divisor_int64 = Int64GetDatum((int64) (1 << count));`
`640`		`- divisor_numeric = DirectFunctionCall1(int8_numeric, divisor_int64);`
	`642`	`+ divisor_numeric = DirectFunctionCall1(int8_numeric, divisor);`
`641`	`643`	`result = DirectFunctionCall2(numeric_div_trunc, d, divisor_numeric);`
`642`	`644`	`return DatumGetNumeric(result);`
`643`	`645`	`}`
`@@ -660,8 +662,8 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)`
`660`	`662`	`else`
`661`	`663`	`{`
`662`	`664`	`/* 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);`
`665`	`667`
`666`	`668`	`if (numeric_is_less(numeric_absolute(size), limit2))`
`667`	`669`	`{`
`@@ -670,17 +672,18 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)`
`670`	`672`	`}`
`671`	`673`	`else`
`672`	`674`	`{`
`673`		`- /* size >>= 10 */`
`674`		`- size = numeric_shift_right(size, 10);`
	`675`	`+ /* size /= (1 << 10) */`
	`676`	`+ size = numeric_truncated_divide(size, 1 << 10);`
	`677`	`+`
`675`	`678`	`if (numeric_is_less(numeric_absolute(size), limit2))`
`676`	`679`	`{`
`677`	`680`	`size = numeric_half_rounded(size);`
`678`	`681`	`result = psprintf("%s MB", numeric_to_cstring(size));`
`679`	`682`	`}`
`680`	`683`	`else`
`681`	`684`	`{`
`682`		`- /* size >>= 10 */`
`683`		`- size = numeric_shift_right(size, 10);`
	`685`	`+ /* size /= (1 << 10) */`
	`686`	`+ size = numeric_truncated_divide(size, 1 << 10);`
`684`	`687`
`685`	`688`	`if (numeric_is_less(numeric_absolute(size), limit2))`
`686`	`689`	`{`
`@@ -689,8 +692,8 @@ pg_size_pretty_numeric(PG_FUNCTION_ARGS)`
`689`	`692`	`}`
`690`	`693`	`else`
`691`	`694`	`{`
`692`		`- /* size >>= 10 */`
`693`		`- size = numeric_shift_right(size, 10);`
	`695`	`+ /* size /= (1 << 10) */`
	`696`	`+ size = numeric_truncated_divide(size, 1 << 10);`
`694`	`697`	`size = numeric_half_rounded(size);`
`695`	`698`	`result = psprintf("%s TB", numeric_to_cstring(size));`
`696`	`699`	`}`