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
