Thread: 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
Attachment
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": 80ms > > This 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
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": 80ms >> >> This 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
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
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
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
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
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
Attachment
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
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
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
On Wed, Aug 27, 2014 at 10:59 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
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.
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.
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
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
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