In the "redesign checkpoint_segments" patch, Robert suggested keeping
the settings' base unit as "number of segments", but allow conversions
from MB, GB etc. I started looking into that and found that adding a new
unit to guc.c is quite messy. The conversions are done with complicated
if-switch-case constructs.
Attached is a patch to refactor that, making the conversions
table-driven. This doesn't change functionality, it just makes the code
nicer.
Any objections?
- Heikki
From a053c61c333687224d33a18a2a299c4dc2eb6bfe Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 13 Feb 2015 15:24:50 +0200
Subject: [PATCH 1/1] Refactor unit conversions code in guc.c.
Replace the if-switch-case constructs with two conversion tables,
containing all the supported conversions between human-readable unit
strings and the base units used in GUC variables. This makes the code
easier to read, and makes adding new units simpler.
---
src/backend/utils/misc/guc.c | 425 +++++++++++++++++++------------------------
src/include/utils/guc.h | 2 +
2 files changed, 188 insertions(+), 239 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9572777..59e25af 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -97,20 +97,6 @@
#define CONFIG_EXEC_PARAMS_NEW "global/config_exec_params.new"
#endif
-#define KB_PER_MB (1024)
-#define KB_PER_GB (1024*1024)
-#define KB_PER_TB (1024*1024*1024)
-
-#define MS_PER_S 1000
-#define S_PER_MIN 60
-#define MS_PER_MIN (1000 * 60)
-#define MIN_PER_H 60
-#define S_PER_H (60 * 60)
-#define MS_PER_H (1000 * 60 * 60)
-#define MIN_PER_D (60 * 24)
-#define S_PER_D (60 * 60 * 24)
-#define MS_PER_D (1000 * 60 * 60 * 24)
-
/*
* Precision with which REAL type guc values are to be printed for GUC
* serialization.
@@ -666,6 +652,88 @@ const char *const config_type_names[] =
/* PGC_ENUM */ "enum"
};
+/*
+ * Unit conversions tables.
+ *
+ * There are two tables, one for memory units, and another for time units.
+ * For each supported conversion from one unit to another, we have an entry
+ * in the conversion table.
+ *
+ * To keep things simple, and to avoid intermediate-value overflows,
+ * conversions are never chained. There needs to be a direct conversion
+ * between all units.
+ *
+ * The conversions from each base unit must be kept in order from greatest
+ * to smallest unit; convert_from_base_unit() relies on that. (The order of
+ * the base units does not matter.)
+ */
+#define MAX_UNIT_LEN 3 /* length of longest recognized unit string */
+
+typedef struct
+{
+ char unit[MAX_UNIT_LEN + 1]; /* unit, as a string, like "kB" or "min" */
+ int base_unit; /* GUC_UNIT_XXX */
+ int multiplier; /* If positive, multiply the value with this for
+ * unit -> base_unit conversion. If negative,
+ * divide (with the absolute value) */
+} unit_conversion;
+
+/* Ensure that the constants in the tables don't overflow or underflow */
+#if BLCKSZ < 1024 || BLCKSZ > (1024*1024)
+#error BLCKSZ must be between 1KB and 1MB
+#endif
+#if XLOG_BLCKSZ < 1024 || XLOG_BLCKSZ > (1024*1024)
+#error XLOG_BLCKSZ must be between 1KB and 1MB
+#endif
+
+static const char *memory_units_hint =
+ gettext_noop("Valid units for this parameter are \"kB\", \"MB\", \"GB\", and \"TB\".");
+
+static const unit_conversion memory_unit_conversion_table[] =
+{
+ { "TB", GUC_UNIT_KB, 1024*1024*1024 },
+ { "GB", GUC_UNIT_KB, 1024*1024 },
+ { "MB", GUC_UNIT_KB, 1024 },
+ { "kB", GUC_UNIT_KB, 1 },
+
+ { "TB", GUC_UNIT_BLOCKS, (1024*1024*1024) / (BLCKSZ / 1024) },
+ { "GB", GUC_UNIT_BLOCKS, (1024*1024) / (BLCKSZ / 1024) },
+ { "MB", GUC_UNIT_BLOCKS, 1024 / (BLCKSZ / 1024) },
+ { "kB", GUC_UNIT_BLOCKS, -(BLCKSZ / 1024) },
+
+ { "TB", GUC_UNIT_XBLOCKS, (1024*1024*1024) / (XLOG_BLCKSZ / 1024) },
+ { "GB", GUC_UNIT_XBLOCKS, (1024*1024) / (XLOG_BLCKSZ / 1024) },
+ { "MB", GUC_UNIT_XBLOCKS, 1024 / (XLOG_BLCKSZ / 1024) },
+ { "kB", GUC_UNIT_XBLOCKS, -(XLOG_BLCKSZ / 1024) },
+
+ { "" } /* end of table marker */
+};
+
+static const char *time_units_hint =
+ gettext_noop("Valid units for this parameter are \"ms\", \"s\", \"min\", \"h\", and \"d\".");
+
+static const unit_conversion time_unit_conversion_table[] =
+{
+ { "d", GUC_UNIT_MS, 1000 * 60 * 60 * 24 },
+ { "h", GUC_UNIT_MS, 1000 * 60 * 60 },
+ { "min", GUC_UNIT_MS, 1000 * 60},
+ { "s", GUC_UNIT_MS, 1000 },
+ { "ms", GUC_UNIT_MS, 1 },
+
+ { "d", GUC_UNIT_S, 60 * 60 * 24 },
+ { "h", GUC_UNIT_S, 60 * 60 },
+ { "min", GUC_UNIT_S, 60 },
+ { "s", GUC_UNIT_S, 1 },
+ { "ms", GUC_UNIT_S, -1000 },
+
+ { "d", GUC_UNIT_MIN, 60 * 24 },
+ { "h", GUC_UNIT_MIN, 60 },
+ { "min", GUC_UNIT_MIN, 1 },
+ { "s", GUC_UNIT_MIN, -60 },
+ { "ms", GUC_UNIT_MIN, -1000 * 60 },
+
+ { "" } /* end of table marker */
+};
/*
* Contents of GUC tables
@@ -5018,6 +5086,85 @@ ReportGUCOption(struct config_generic * record)
}
/*
+ * Convert a value from one of the human-friendly units ("kB", "min" etc.)
+ * to a given base unit. 'value' and 'unit' are the input value and unit to
+ * convert from. The value after conversion to 'base_unit' is stored in
+ * *base_value.
+ *
+ * Returns true on success, or false if the input unit is not recognized.
+ */
+static bool
+convert_to_base_unit(int64 value, const char *unit,
+ int base_unit, int64 *base_value)
+{
+ const unit_conversion *table;
+ int i;
+
+ if (base_unit & GUC_UNIT_MEMORY)
+ table = memory_unit_conversion_table;
+ else
+ table = time_unit_conversion_table;
+
+ for (i = 0; *table[i].unit; i++)
+ {
+ if (base_unit == table[i].base_unit &&
+ strcmp(unit, table[i].unit) == 0)
+ {
+ if (table[i].multiplier < 0)
+ *base_value = value / (-table[i].multiplier);
+ else
+ *base_value = value * table[i].multiplier;
+ return true;
+ }
+ }
+ return false;
+}
+
+/*
+ * Convert a value in some base unit to a human-friendly unit.
+ * The output unit is chosen so that it's the greatest unit that can represent
+ * the value without loss. For example, if the base unit is GUC_UNIT_KB, 1024
+ * converted to 1 MB, but 1025 is represented as 1025 kB.
+ */
+static void
+convert_from_base_unit(int64 base_value, int base_unit,
+ int64 *value, const char **unit)
+{
+ const unit_conversion *table;
+ int i;
+
+ *unit = NULL;
+
+ if (base_unit & GUC_UNIT_MEMORY)
+ table = memory_unit_conversion_table;
+ else
+ table = time_unit_conversion_table;
+
+ for (i = 0; *table[i].unit; i++)
+ {
+ if (base_unit == table[i].base_unit)
+ {
+ /* accept the first conversion that divides the value evenly */
+ if (table[i].multiplier < 0)
+ {
+ *value = base_value * (-table[i].multiplier);
+ *unit = table[i].unit;
+ break;
+ }
+ else if (base_value % table[i].multiplier == 0)
+ {
+ *value = base_value / table[i].multiplier;
+ *unit = table[i].unit;
+ break;
+ }
+ }
+ }
+
+ Assert(*unit != NULL);
+}
+
+
+/*
* Try to parse value as an integer. The accepted formats are the
* usual decimal, octal, or hexadecimal formats, optionally followed by
* a unit name if "flags" indicates a unit is allowed.
@@ -5060,170 +5207,36 @@ parse_int(const char *value, int *result, int flags, const char **hintmsg)
/* Handle possible unit */
if (*endptr != '\0')
{
- /*
- * Note: the multiple-switch coding technique here is a bit tedious,
- * but seems necessary to avoid intermediate-value overflows.
- */
- if (flags & GUC_UNIT_MEMORY)
- {
- /* Set hint for use if no match or trailing garbage */
- if (hintmsg)
- *hintmsg = gettext_noop("Valid units for this parameter are \"kB\", \"MB\", \"GB\", and \"TB\".");
+ char unit[MAX_UNIT_LEN + 1];
+ int unitlen;
+ bool converted = false;
-#if BLCKSZ < 1024 || BLCKSZ > (1024*1024)
-#error BLCKSZ must be between 1KB and 1MB
-#endif
-#if XLOG_BLCKSZ < 1024 || XLOG_BLCKSZ > (1024*1024)
-#error XLOG_BLCKSZ must be between 1KB and 1MB
-#endif
+ if ((flags & GUC_UNIT) == 0)
+ return false; /* this setting does not accept a unit */
- if (strncmp(endptr, "kB", 2) == 0)
- {
- endptr += 2;
- switch (flags & GUC_UNIT_MEMORY)
- {
- case GUC_UNIT_BLOCKS:
- val /= (BLCKSZ / 1024);
- break;
- case GUC_UNIT_XBLOCKS:
- val /= (XLOG_BLCKSZ / 1024);
- break;
- }
- }
- else if (strncmp(endptr, "MB", 2) == 0)
- {
- endptr += 2;
- switch (flags & GUC_UNIT_MEMORY)
- {
- case GUC_UNIT_KB:
- val *= KB_PER_MB;
- break;
- case GUC_UNIT_BLOCKS:
- val *= KB_PER_MB / (BLCKSZ / 1024);
- break;
- case GUC_UNIT_XBLOCKS:
- val *= KB_PER_MB / (XLOG_BLCKSZ / 1024);
- break;
- }
- }
- else if (strncmp(endptr, "GB", 2) == 0)
- {
- endptr += 2;
- switch (flags & GUC_UNIT_MEMORY)
- {
- case GUC_UNIT_KB:
- val *= KB_PER_GB;
- break;
- case GUC_UNIT_BLOCKS:
- val *= KB_PER_GB / (BLCKSZ / 1024);
- break;
- case GUC_UNIT_XBLOCKS:
- val *= KB_PER_GB / (XLOG_BLCKSZ / 1024);
- break;
- }
- }
- else if (strncmp(endptr, "TB", 2) == 0)
- {
- endptr += 2;
- switch (flags & GUC_UNIT_MEMORY)
- {
- case GUC_UNIT_KB:
- val *= KB_PER_TB;
- break;
- case GUC_UNIT_BLOCKS:
- val *= KB_PER_TB / (BLCKSZ / 1024);
- break;
- case GUC_UNIT_XBLOCKS:
- val *= KB_PER_TB / (XLOG_BLCKSZ / 1024);
- break;
- }
- }
- }
- else if (flags & GUC_UNIT_TIME)
- {
- /* Set hint for use if no match or trailing garbage */
- if (hintmsg)
- *hintmsg = gettext_noop("Valid units for this parameter are \"ms\", \"s\", \"min\", \"h\", and \"d\".");
+ unitlen = 0;
+ while (*endptr != '\0' && unitlen < MAX_UNIT_LEN)
+ unit[unitlen++] = *(endptr++);
+ unit[unitlen] = '\0';
- if (strncmp(endptr, "ms", 2) == 0)
- {
- endptr += 2;
- switch (flags & GUC_UNIT_TIME)
- {
- case GUC_UNIT_S:
- val /= MS_PER_S;
- break;
- case GUC_UNIT_MIN:
- val /= MS_PER_MIN;
- break;
- }
- }
- else if (strncmp(endptr, "s", 1) == 0)
- {
- endptr += 1;
- switch (flags & GUC_UNIT_TIME)
- {
- case GUC_UNIT_MS:
- val *= MS_PER_S;
- break;
- case GUC_UNIT_MIN:
- val /= S_PER_MIN;
- break;
- }
- }
- else if (strncmp(endptr, "min", 3) == 0)
- {
- endptr += 3;
- switch (flags & GUC_UNIT_TIME)
- {
- case GUC_UNIT_MS:
- val *= MS_PER_MIN;
- break;
- case GUC_UNIT_S:
- val *= S_PER_MIN;
- break;
- }
- }
- else if (strncmp(endptr, "h", 1) == 0)
- {
- endptr += 1;
- switch (flags & GUC_UNIT_TIME)
- {
- case GUC_UNIT_MS:
- val *= MS_PER_H;
- break;
- case GUC_UNIT_S:
- val *= S_PER_H;
- break;
- case GUC_UNIT_MIN:
- val *= MIN_PER_H;
- break;
- }
- }
- else if (strncmp(endptr, "d", 1) == 0)
- {
- endptr += 1;
- switch (flags & GUC_UNIT_TIME)
- {
- case GUC_UNIT_MS:
- val *= MS_PER_D;
- break;
- case GUC_UNIT_S:
- val *= S_PER_D;
- break;
- case GUC_UNIT_MIN:
- val *= MIN_PER_D;
- break;
- }
- }
- }
+ converted = convert_to_base_unit(val, unit, (flags & GUC_UNIT), &val);
/* allow whitespace after unit */
while (isspace((unsigned char) *endptr))
endptr++;
- if (*endptr != '\0')
- return false; /* appropriate hint, if any, already set */
+ if (!converted || *endptr != '\0')
+ {
+ /* invalid unit, or garbage after the unit; set hint and fail. */
+ if (hintmsg)
+ {
+ if (flags & GUC_UNIT_MEMORY)
+ *hintmsg = memory_units_hint;
+ else
+ *hintmsg = time_units_hint;
+ }
+ return false;
+ }
/* Check for overflow due to units conversion */
if (val != (int64) ((int32) val))
@@ -8096,76 +8109,10 @@ _ShowOption(struct config_generic * record, bool use_units)
int64 result = *conf->variable;
const char *unit;
- if (use_units && result > 0 &&
- (record->flags & GUC_UNIT_MEMORY))
- {
- switch (record->flags & GUC_UNIT_MEMORY)
- {
- case GUC_UNIT_BLOCKS:
- result *= BLCKSZ / 1024;
- break;
- case GUC_UNIT_XBLOCKS:
- result *= XLOG_BLCKSZ / 1024;
- break;
- }
-
- if (result % KB_PER_TB == 0)
- {
- result /= KB_PER_TB;
- unit = "TB";
- }
- else if (result % KB_PER_GB == 0)
- {
- result /= KB_PER_GB;
- unit = "GB";
- }
- else if (result % KB_PER_MB == 0)
- {
- result /= KB_PER_MB;
- unit = "MB";
- }
- else
- {
- unit = "kB";
- }
- }
- else if (use_units && result > 0 &&
- (record->flags & GUC_UNIT_TIME))
+ if (use_units && result > 0 && (record->flags & GUC_UNIT))
{
- switch (record->flags & GUC_UNIT_TIME)
- {
- case GUC_UNIT_S:
- result *= MS_PER_S;
- break;
- case GUC_UNIT_MIN:
- result *= MS_PER_MIN;
- break;
- }
-
- if (result % MS_PER_D == 0)
- {
- result /= MS_PER_D;
- unit = "d";
- }
- else if (result % MS_PER_H == 0)
- {
- result /= MS_PER_H;
- unit = "h";
- }
- else if (result % MS_PER_MIN == 0)
- {
- result /= MS_PER_MIN;
- unit = "min";
- }
- else if (result % MS_PER_S == 0)
- {
- result /= MS_PER_S;
- unit = "s";
- }
- else
- {
- unit = "ms";
- }
+ convert_from_base_unit(result, record->flags & GUC_UNIT,
+ &result, &unit);
}
else
unit = "";
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 717f46b..9a9a7a0 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -212,6 +212,8 @@ typedef enum
#define GUC_UNIT_MIN 0x4000 /* value is in minutes */
#define GUC_UNIT_TIME 0x7000 /* mask for MS, S, MIN */
+#define GUC_UNIT (GUC_UNIT_MEMORY | GUC_UNIT_TIME)
+
#define GUC_NOT_WHILE_SEC_REST 0x8000 /* can't set if security restricted */
#define GUC_DISALLOW_IN_AUTO_FILE 0x00010000 /* can't set in
* PG_AUTOCONF_FILENAME */
--
2.1.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
