Thread: Specifying the unit in storage parameter

Specifying the unit in storage parameter

From
Fujii Masao
Date:
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

Re: Specifying the unit in storage parameter

From
Alvaro Herrera
Date:
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



Re: Specifying the unit in storage parameter

From
Fujii Masao
Date:
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



Re: Specifying the unit in storage parameter

From
Alvaro Herrera
Date:
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



Re: Specifying the unit in storage parameter

From
Fujii Masao
Date:
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



Re: Specifying the unit in storage parameter

From
Josh Berkus
Date:
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



Re: Specifying the unit in storage parameter

From
Michael Paquier
Date:
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



Re: Specifying the unit in storage parameter

From
Fujii Masao
Date:
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

Re: Specifying the unit in storage parameter

From
Alvaro Herrera
Date:
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



Re: Specifying the unit in storage parameter

From
Fujii Masao
Date:
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



Re: Specifying the unit in storage parameter

From
Alvaro Herrera
Date:
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



Re: Specifying the unit in storage parameter

From
Michael Paquier
Date:
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

Re: Specifying the unit in storage parameter

From
Fujii Masao
Date:
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



Re: Specifying the unit in storage parameter

From
Michael Paquier
Date:
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



Re: Specifying the unit in storage parameter

From
Alvaro Herrera
Date:
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