On Thu, Aug 21, 2014 at 4:20 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, Aug 8, 2014 at 12:32 PM, Fujii Masao <masao.fu...@gmail.com> wrote: >> This is not user-friendly. I'd like to propose the attached patch which >> introduces the infrastructure which allows us to specify the unit when >> setting INTEGER storage parameter like autovacuum_vacuum_cost_delay. >> Comment? Review? > This patch makes autovacuum_vacuum_cost_delay more consistent with > what is at server level. So +1.
Thanks for reviewing the patch! > Looking at the patch, the parameter "fillfactor" in the category > RELOPT_KIND_HEAP (the first element in intRelOpts of reloptions.c) is > not updated with the new field. It is only a one-line change. > @@ -97,7 +97,7 @@ static relopt_int intRelOpts[] = > "Packs table pages only to this percentage", > RELOPT_KIND_HEAP > }, > - HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100 > + HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100, 0 > }, Oh, good catch. I wonder why I did such a mistake... Attached is the updated version of the patch. > Except that, I tested as well the patch and it works as expected. The > documentation, as well as the regression tests remain untouched, but I > guess that this is fine (not seeing similar tests in regressions, and > documentation does not specify the unit for a given parameter). I think that it's worth adding the regression test for this feature. Attached patch updates the regression test. Regards, -- Fujii Masao
*** a/src/backend/access/common/reloptions.c --- b/src/backend/access/common/reloptions.c *************** *** 97,103 **** static relopt_int intRelOpts[] = "Packs table pages only to this percentage", RELOPT_KIND_HEAP }, ! HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100 }, { { --- 97,103 ---- "Packs table pages only to this percentage", RELOPT_KIND_HEAP }, ! HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100, 0 }, { { *************** *** 105,111 **** static relopt_int intRelOpts[] = "Packs btree index pages only to this percentage", RELOPT_KIND_BTREE }, ! BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100 }, { { --- 105,111 ---- "Packs btree index pages only to this percentage", RELOPT_KIND_BTREE }, ! BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100, 0 }, { { *************** *** 113,119 **** static relopt_int intRelOpts[] = "Packs hash index pages only to this percentage", RELOPT_KIND_HASH }, ! HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100 }, { { --- 113,119 ---- "Packs hash index pages only to this percentage", RELOPT_KIND_HASH }, ! HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100, 0 }, { { *************** *** 121,127 **** static relopt_int intRelOpts[] = "Packs gist index pages only to this percentage", RELOPT_KIND_GIST }, ! GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100 }, { { --- 121,127 ---- "Packs gist index pages only to this percentage", RELOPT_KIND_GIST }, ! GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100, 0 }, { { *************** *** 129,135 **** static relopt_int intRelOpts[] = "Packs spgist index pages only to this percentage", RELOPT_KIND_SPGIST }, ! SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100 }, { { --- 129,135 ---- "Packs spgist index pages only to this percentage", RELOPT_KIND_SPGIST }, ! SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100, 0 }, { { *************** *** 137,143 **** static relopt_int intRelOpts[] = "Minimum number of tuple updates or deletes prior to vacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 0, INT_MAX }, { { --- 137,143 ---- "Minimum number of tuple updates or deletes prior to vacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 0, INT_MAX, 0 }, { { *************** *** 145,151 **** static relopt_int intRelOpts[] = "Minimum number of tuple inserts, updates or deletes prior to analyze", RELOPT_KIND_HEAP }, ! -1, 0, INT_MAX }, { { --- 145,151 ---- "Minimum number of tuple inserts, updates or deletes prior to analyze", RELOPT_KIND_HEAP }, ! -1, 0, INT_MAX, 0 }, { { *************** *** 153,159 **** static relopt_int intRelOpts[] = "Vacuum cost delay in milliseconds, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 0, 100 }, { { --- 153,159 ---- "Vacuum cost delay in milliseconds, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 0, 100, GUC_UNIT_MS }, { { *************** *** 161,167 **** static relopt_int intRelOpts[] = "Vacuum cost amount available before napping, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 1, 10000 }, { { --- 161,167 ---- "Vacuum cost amount available before napping, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 1, 10000, 0 }, { { *************** *** 169,175 **** static relopt_int intRelOpts[] = "Minimum age at which VACUUM should freeze a table row, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 0, 1000000000 }, { { --- 169,175 ---- "Minimum age at which VACUUM should freeze a table row, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 0, 1000000000, 0 }, { { *************** *** 177,183 **** static relopt_int intRelOpts[] = "Minimum multixact age at which VACUUM should freeze a row multixact's, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 0, 1000000000 }, { { --- 177,183 ---- "Minimum multixact age at which VACUUM should freeze a row multixact's, for autovacuum", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 0, 1000000000, 0 }, { { *************** *** 185,191 **** static relopt_int intRelOpts[] = "Age at which to autovacuum a table to prevent transaction ID wraparound", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 100000000, 2000000000 }, { { --- 185,191 ---- "Age at which to autovacuum a table to prevent transaction ID wraparound", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 100000000, 2000000000, 0 }, { { *************** *** 193,213 **** static relopt_int intRelOpts[] = "Multixact age at which to autovacuum a table to prevent multixact wraparound", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 100000000, 2000000000 }, { { "autovacuum_freeze_table_age", "Age at which VACUUM should perform a full table sweep to freeze row versions", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST ! }, -1, 0, 2000000000 }, { { "autovacuum_multixact_freeze_table_age", "Age of multixact at which VACUUM should perform a full table sweep to freeze row versions", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST ! }, -1, 0, 2000000000 }, /* list terminator */ --- 193,213 ---- "Multixact age at which to autovacuum a table to prevent multixact wraparound", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST }, ! -1, 100000000, 2000000000, 0 }, { { "autovacuum_freeze_table_age", "Age at which VACUUM should perform a full table sweep to freeze row versions", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST ! }, -1, 0, 2000000000, 0 }, { { "autovacuum_multixact_freeze_table_age", "Age of multixact at which VACUUM should perform a full table sweep to freeze row versions", RELOPT_KIND_HEAP | RELOPT_KIND_TOAST ! }, -1, 0, 2000000000, 0 }, /* list terminator */ *************** *** 503,509 **** add_bool_reloption(bits32 kinds, char *name, char *desc, bool default_val) */ void add_int_reloption(bits32 kinds, char *name, char *desc, int default_val, ! int min_val, int max_val) { relopt_int *newoption; --- 503,509 ---- */ void add_int_reloption(bits32 kinds, char *name, char *desc, int default_val, ! int min_val, int max_val, int flags_val) { relopt_int *newoption; *************** *** 512,517 **** add_int_reloption(bits32 kinds, char *name, char *desc, int default_val, --- 512,518 ---- newoption->default_val = default_val; newoption->min = min_val; newoption->max = max_val; + newoption->flags = flags_val; add_reloption((relopt_gen *) newoption); } *************** *** 1000,1011 **** parse_one_reloption(relopt_value *option, char *text_str, int text_len, case RELOPT_TYPE_INT: { relopt_int *optint = (relopt_int *) option->gen; ! parsed = parse_int(value, &option->values.int_val, 0, NULL); if (validate && !parsed) ereport(ERROR, (errmsg("invalid value for integer option \"%s\": %s", ! option->gen->name, value))); if (validate && (option->values.int_val < optint->min || option->values.int_val > optint->max)) ereport(ERROR, --- 1001,1015 ---- case RELOPT_TYPE_INT: { relopt_int *optint = (relopt_int *) option->gen; + const char *hintmsg; ! parsed = parse_int(value, &option->values.int_val, ! optint->flags, &hintmsg); if (validate && !parsed) ereport(ERROR, (errmsg("invalid value for integer option \"%s\": %s", ! option->gen->name, value), ! hintmsg ? errhint("%s", _(hintmsg)) : 0)); if (validate && (option->values.int_val < optint->min || option->values.int_val > optint->max)) ereport(ERROR, *** a/src/include/access/reloptions.h --- b/src/include/access/reloptions.h *************** *** 92,97 **** typedef struct relopt_int --- 92,98 ---- int default_val; int min; int max; + int flags; } relopt_int; typedef struct relopt_real *************** *** 244,250 **** extern relopt_kind add_reloption_kind(void); extern void add_bool_reloption(bits32 kinds, char *name, char *desc, bool default_val); extern void add_int_reloption(bits32 kinds, char *name, char *desc, ! int default_val, int min_val, int max_val); extern void add_real_reloption(bits32 kinds, char *name, char *desc, double default_val, double min_val, double max_val); extern void add_string_reloption(bits32 kinds, char *name, char *desc, --- 245,251 ---- extern void add_bool_reloption(bits32 kinds, char *name, char *desc, bool default_val); extern void add_int_reloption(bits32 kinds, char *name, char *desc, ! int default_val, int min_val, int max_val, int flags_val); extern void add_real_reloption(bits32 kinds, char *name, char *desc, double default_val, double min_val, double max_val); extern void add_string_reloption(bits32 kinds, char *name, char *desc, *** a/src/test/regress/expected/alter_table.out --- b/src/test/regress/expected/alter_table.out *************** *** 1811,1816 **** Check constraints: --- 1811,1830 ---- "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision) Inherits: test_inh_check + -- Set a storage parameter with unit + CREATE TABLE test_param_unit (a text) WITH (autovacuum_vacuum_cost_delay = '80ms'); + ALTER TABLE test_param_unit SET (autovacuum_vacuum_cost_delay = '3min'); + ERROR: value 3min out of bounds for option "autovacuum_vacuum_cost_delay" + DETAIL: Valid values are between "0" and "100". + ALTER TABLE test_param_unit SET (autovacuum_analyze_threshold = '3min'); -- fails + ERROR: invalid value for integer option "autovacuum_analyze_threshold": 3min + \d+ test_param_unit + Table "public.test_param_unit" + Column | Type | Modifiers | Storage | Stats target | Description + --------+------+-----------+----------+--------------+------------- + a | text | | extended | | + Options: autovacuum_vacuum_cost_delay=80ms + -- -- lock levels -- *** a/src/test/regress/sql/alter_table.sql --- b/src/test/regress/sql/alter_table.sql *************** *** 1254,1259 **** ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric; --- 1254,1265 ---- \d test_inh_check \d test_inh_check_child + -- Set a storage parameter with unit + CREATE TABLE test_param_unit (a text) WITH (autovacuum_vacuum_cost_delay = '80ms'); + ALTER TABLE test_param_unit SET (autovacuum_vacuum_cost_delay = '3min'); + ALTER TABLE test_param_unit SET (autovacuum_analyze_threshold = '3min'); -- fails + \d+ test_param_unit + -- -- lock levels --
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers