Specifying the unit in storage parameter
Hi,
We can specify the unit when setting autovacuum_vacuum_cost_delay
GUC as follows.
ALTER SYSTEM SET autovacuum_vacuum_cost_delay TO '80ms';
OTOH we cannot specify the unit when setting autovacuum_vacuum_cost_delay
as storage parameter as follows.
CREATE TABLE test (col1 int) WITH (autovacuum_vacuum_cost_delay = '80ms');
ERROR: invalid value for integer option
"autovacuum_vacuum_cost_delay": 80ms
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?
Regards,
--
Fujii Masao
Attachments:
allow_to_specify_unit_in_reloption_v1.patchtext/x-patch; charset=US-ASCII; name=allow_to_specify_unit_in_reloption_v1.patchDownload
*** a/src/backend/access/common/reloptions.c
--- b/src/backend/access/common/reloptions.c
***************
*** 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);
}
***************
*** 1003,1014 **** 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,
--- 1004,1018 ----
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,
Fujii Masao wrote:
Hi,
We can specify the unit when setting autovacuum_vacuum_cost_delay
GUC as follows.ALTER SYSTEM SET autovacuum_vacuum_cost_delay TO '80ms';
OTOH we cannot specify the unit when setting autovacuum_vacuum_cost_delay
as storage parameter as follows.CREATE TABLE test (col1 int) WITH (autovacuum_vacuum_cost_delay = '80ms');
ERROR: invalid value for integer option
"autovacuum_vacuum_cost_delay": 80msThis is not user-friendly.
No disagreement here.
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?
Hm, what's with the parse_int signature change and the hintmsg thing?
Is it just me or the patch is incomplete?
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 8, 2014 at 12:56 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Fujii Masao wrote:
Hi,
We can specify the unit when setting autovacuum_vacuum_cost_delay
GUC as follows.ALTER SYSTEM SET autovacuum_vacuum_cost_delay TO '80ms';
OTOH we cannot specify the unit when setting autovacuum_vacuum_cost_delay
as storage parameter as follows.CREATE TABLE test (col1 int) WITH (autovacuum_vacuum_cost_delay = '80ms');
ERROR: invalid value for integer option
"autovacuum_vacuum_cost_delay": 80msThis is not user-friendly.
No disagreement here.
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?Hm, what's with the parse_int signature change and the hintmsg thing?
Is it just me or the patch is incomplete?
Sorry, probably I failed to see your point. You mean that the signature
of parse_int needs to be changed so that it includes the hintmsg as the
argument? If yes, there is no problem. Both signature and function body
of parse_int has already included the hingmsg as the argument so far.
Am I missing something?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fujii Masao wrote:
On Fri, Aug 8, 2014 at 12:56 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Hm, what's with the parse_int signature change and the hintmsg thing?
Is it just me or the patch is incomplete?Sorry, probably I failed to see your point. You mean that the signature
of parse_int needs to be changed so that it includes the hintmsg as the
argument? If yes, there is no problem. Both signature and function body
of parse_int has already included the hingmsg as the argument so far.
Am I missing something?
I just mean that the parse_int function body is not touched by your
patch, unless I am failing to see something.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 8, 2014 at 2:12 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Fujii Masao wrote:
On Fri, Aug 8, 2014 at 12:56 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Hm, what's with the parse_int signature change and the hintmsg thing?
Is it just me or the patch is incomplete?Sorry, probably I failed to see your point. You mean that the signature
of parse_int needs to be changed so that it includes the hintmsg as the
argument? If yes, there is no problem. Both signature and function body
of parse_int has already included the hingmsg as the argument so far.
Am I missing something?I just mean that the parse_int function body is not touched by your
patch, unless I am failing to see something.
Yes, my patch doesn't change the parse_int function at all because I didn't
think such change is required for the purpose (i.e., just allows us to specify
the unit in the setting of storage parameters). But, you might find the
reason why it needs to be changed?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08/07/2014 08:32 PM, Fujii Masao 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?
No review, but thank you for doing this!
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM2837424d5c6b2945008aba62f780637fed28ea96ebf7013da1c33a9738413dcfa1032725a00e86c0ffc87cfe584fccb8@asav-1.01.com
On Fri, Aug 8, 2014 at 12:32 PM, Fujii Masao <masao.fujii@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.
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
},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).
Regards,
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 21, 2014 at 4:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Fri, Aug 8, 2014 at 12:32 PM, Fujii Masao <masao.fujii@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
Attachments:
allow_to_specify_unit_in_reloption_v2.patchtext/x-diff; charset=US-ASCII; name=allow_to_specify_unit_in_reloption_v2.patchDownload
*** 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
--
Fujii Masao wrote:
On Thu, Aug 21, 2014 at 4:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
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...
Uninitialized elements at end of struct are filled with zeroes. We do
have other examples of this -- for instance, config_generic in the guc.c
tables are almost always only 5 members long even though the struct is
quite a bit longer than that. Most entries do not even have "flags" set.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 26, 2014 at 3:27 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Fujii Masao wrote:
On Thu, Aug 21, 2014 at 4:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote: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...
Uninitialized elements at end of struct are filled with zeroes.
Yeah, that's the reason why I could not notice the problem at compile time.
We do
have other examples of this -- for instance, config_generic in the guc.c
tables are almost always only 5 members long even though the struct is
quite a bit longer than that. Most entries do not even have "flags" set.
So you imply that the trailing zero (which the patch adds as flag)
in the reloption struct should be dropped?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fujii Masao wrote:
On Tue, Aug 26, 2014 at 3:27 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:Fujii Masao wrote:
On Thu, Aug 21, 2014 at 4:20 PM, Michael Paquier
<michael.paquier@gmail.com> wrote: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...
Uninitialized elements at end of struct are filled with zeroes.
Yeah, that's the reason why I could not notice the problem at compile time.
Right -- it's not something the compiler would warn you about.
We do
have other examples of this -- for instance, config_generic in the guc.c
tables are almost always only 5 members long even though the struct is
quite a bit longer than that. Most entries do not even have "flags" set.So you imply that the trailing zero (which the patch adds as flag)
in the reloption struct should be dropped?
Not necessarily, because it's harmless. It's there for purely
aesthetical reasons, so it's your choice whether to add it or not.
Having it there is slightly easier on somebody reading the code,
perhaps.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 27, 2014 at 10:59 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
Not necessarily, because it's harmless. It's there for purely
aesthetical reasons, so it's your choice whether to add it or not.
Having it there is slightly easier on somebody reading the code,
perhaps.
On my side, that's up to you Fujii-san. The patch does what it states, I
only think that this extra 0 should be added either everywhere or nowhere.
Not mandatory either: drop test_param_unit in the regression tests after
running the test queries.
Regards,
--
Michael
On Thu, Aug 28, 2014 at 12:55 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Aug 27, 2014 at 10:59 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:Not necessarily, because it's harmless. It's there for purely
aesthetical reasons, so it's your choice whether to add it or not.
Having it there is slightly easier on somebody reading the code,
perhaps.
Agreed.
On my side, that's up to you Fujii-san. The patch does what it states, I
only think that this extra 0 should be added either everywhere or nowhere.
Yep. I added extra 0 everywhere.
Ok, I just applied the patch. Thanks for the review!
Not mandatory either: drop test_param_unit in the regression tests after
running the test queries.
I don't have strong opinion about this. There are many tables which
regression test creates but doesn't drop. But if you strongly think that
the table must be dropped, I'm OK with that.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 28, 2014 at 4:20 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
I don't have strong opinion about this. There are many tables which
regression test creates but doesn't drop. But if you strongly think that
the table must be dropped, I'm OK with that.
This remark is just to limit the amount of trash in the database used
for regression tests. But then if we'd remove everything we would lack
handy material for tests on utilities like database-wide thingies of
the type VACUUM, REINDEX, pg_dump, etc. And we can just drop the
database used for regressions to clean up everything. So that's not
mandatory at all. I tend to always clean up objects in my patches
touching regressions to limit interactions with other tests, but I
guess that's up to the person who wrote the code to decide.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier wrote:
This remark is just to limit the amount of trash in the database used
for regression tests. But then if we'd remove everything we would lack
handy material for tests on utilities like database-wide thingies of
the type VACUUM, REINDEX, pg_dump, etc. And we can just drop the
database used for regressions to clean up everything. So that's not
mandatory at all. I tend to always clean up objects in my patches
touching regressions to limit interactions with other tests, but I
guess that's up to the person who wrote the code to decide.
Leaving lingering objects is not a bad thing, particularly if they have
unusual properties; they enable somebody pg_dump'ing the database which
can be a good test for pg_dump.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers