Re: pgsql: Allow units to be specified in relation option setting value.

Allow units to be specified in relation option setting value.

This introduces an infrastructure which allows us to specify the units
like ms (milliseconds) in integer relation option, like GUC parameter.
Currently only autovacuum_vacuum_cost_delay reloption can accept
the units.

Reviewed by Michael Paquier

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/e23014f3d40f7d2c23bc97207fd28efbe5ba102b

Modified Files
--------------
src/backend/access/common/reloptions.c | 40 ++++++++++++++++-------------
src/include/access/reloptions.h | 3 ++-
src/test/regress/expected/alter_table.out | 14 ++++++++++
src/test/regress/sql/alter_table.sql | 6 +++++
4 files changed, 44 insertions(+), 19 deletions(-)

Hi,

On 2014-08-28 07:15:35 +0000, Fujii Masao wrote:
> Allow units to be specified in relation option setting value.
>
> This introduces an infrastructure which allows us to specify the units
> like ms (milliseconds) in integer relation option, like GUC parameter.
> Currently only autovacuum_vacuum_cost_delay reloption can accept
> the units.

This apparently broke pg_upgrade. I've not checked what's up there.

See
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2014-08-28%2007%3A16%3A04
and many others.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Aug 28, 2014 at 9:11 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> On 2014-08-28 07:15:35 +0000, Fujii Masao wrote:
>> Allow units to be specified in relation option setting value.
>>
>> This introduces an infrastructure which allows us to specify the units
>> like ms (milliseconds) in integer relation option, like GUC parameter.
>> Currently only autovacuum_vacuum_cost_delay reloption can accept
>> the units.
>
> This apparently broke pg_upgrade. I've not checked what's up there.
>
> See
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2014-08-28%2007%3A16%3A04
> and many others.

Oh,, will check.

Regards,

--
Fujii Masao

On Thu, Aug 28, 2014 at 9:28 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> Oh,, will check.

That's a problem with pg_dump not able to put quotes where for a
reloption units are used.
For example this table:
create table test (a int) with (autovacuum_vacuum_cost_delay = '80ms');
Results in the following dump:
CREATE TABLE test (
a integer
)
WITH (autovacuum_vacuum_cost_delay=80ms);

Because of how reloptions is registered in pg_class:
=# select relname,reloptions from pg_class where relname = 'test';
relname | reloptions
---------+-------------------------------------
test | {autovacuum_vacuum_cost_delay=80ms}
(1 row)
Regards,
--
Michael

On Thu, Aug 28, 2014 at 10:51 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Because of how reloptions is registered in pg_class:
The patch attached fixes pg_upgrade by putting quotes when generating
reloptions and it passes check-world. I imagine that having quotes by
default in the value of reloptions in pg_class is the price to pay for
supporting units. If this is not considered as a correct approach,
then reverting the patch would be wiser I guess.
Regards,
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> The patch attached fixes pg_upgrade by putting quotes when generating
> reloptions and it passes check-world. I imagine that having quotes by
> default in the value of reloptions in pg_class is the price to pay for
> supporting units. If this is not considered as a correct approach,
> then reverting the patch would be wiser I guess.

Ugh. I'm not sure what the best solution is, but I don't think I like
that one.

Given that this patch broke both pg_dump and pg_upgrade, I think it should
be reverted for the time being, rather than applying a hasty hack.
Obviously more thought and testing is needed.

regards, tom lane

On 2014-08-28 14:11:33 +0200, Andres Freund wrote:
> Hi,
>
> On 2014-08-28 07:15:35 +0000, Fujii Masao wrote:
> > Allow units to be specified in relation option setting value.
> >
> > This introduces an infrastructure which allows us to specify the units
> > like ms (milliseconds) in integer relation option, like GUC parameter.
> > Currently only autovacuum_vacuum_cost_delay reloption can accept
> > the units.
>
> This apparently broke pg_upgrade. I've not checked what's up there.
>
> See
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2014-08-28%2007%3A16%3A04
> and many others.

The specific error is an unquoted 80ms in WITH
(autovacuum_vacuum_cost_delay=80ms);

Independent of that being fixable with some quoting or such, I'm a bit
doubtful about unconditionally adding the ms here. Won't that make it
unnecessarily hard to get 9.5 dumps into <9.5? I know that we don't make
any promises, but it mostly works nonetheless. And this doesn't seem
worth the breakage.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Aug 28, 2014 at 11:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> The patch attached fixes pg_upgrade by putting quotes when generating
>> reloptions and it passes check-world. I imagine that having quotes by
>> default in the value of reloptions in pg_class is the price to pay for
>> supporting units. If this is not considered as a correct approach,
>> then reverting the patch would be wiser I guess.
>
> Ugh. I'm not sure what the best solution is, but I don't think I like
> that one.

Another approach is to change pg_dump so that it encloses the relopt
values with single quotes. This is the same approach as what
pg_dumpall does for database or role-specific settings. Obvious
drawback of this approach is that it prevents pg_dump with 9.4 or
before from outputting the restorable dump. Maybe we can live with
this because there is no guarantee that older version of pg_dump can
work properly with newer major version of server. But maybe
someone cannot live with that. Not sure.

Further other approach is to change the reloptions code so that it
always stores the plain value without the units (i.e., 1000 is stored
if 1s is specified in autovacuum_vacuum_cost_delay)in pg_class.
This approach doesn't prevent older version of pg_dump from taking
the dump from v9.5 server. One drawback of this approach is that
reloption values are always stored without the units, which might
make it a bit hard for a user to see the reloption values from pg_class.

Regards,

--
Fujii Masao

On Thu, Aug 28, 2014 at 12:22 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Aug 28, 2014 at 11:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>>> The patch attached fixes pg_upgrade by putting quotes when generating
>>> reloptions and it passes check-world. I imagine that having quotes by
>>> default in the value of reloptions in pg_class is the price to pay for
>>> supporting units. If this is not considered as a correct approach,
>>> then reverting the patch would be wiser I guess.
>>
>> Ugh. I'm not sure what the best solution is, but I don't think I like
>> that one.
>
> Another approach is to change pg_dump so that it encloses the relopt
> values with single quotes. This is the same approach as what
> pg_dumpall does for database or role-specific settings. Obvious
> drawback of this approach is that it prevents pg_dump with 9.4 or
> before from outputting the restorable dump. Maybe we can live with
> this because there is no guarantee that older version of pg_dump can
> work properly with newer major version of server. But maybe
> someone cannot live with that. Not sure.

To me, this doesn't seem nearly important enough to justify breaking
pg_dump compatibility. AAUI, this is just a cosmetic improvement, so
we shouldn't break functional things for that.

> Further other approach is to change the reloptions code so that it
> always stores the plain value without the units (i.e., 1000 is stored
> if 1s is specified in autovacuum_vacuum_cost_delay)in pg_class.
> This approach doesn't prevent older version of pg_dump from taking
> the dump from v9.5 server. One drawback of this approach is that
> reloption values are always stored without the units, which might
> make it a bit hard for a user to see the reloption values from pg_class.

This seems like the way to go.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Aug 28, 2014 at 12:22 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> Another approach is to change pg_dump so that it encloses the relopt
>> values with single quotes. This is the same approach as what
>> pg_dumpall does for database or role-specific settings.

> To me, this doesn't seem nearly important enough to justify breaking
> pg_dump compatibility. AAUI, this is just a cosmetic improvement, so
> we shouldn't break functional things for that.

Indeed. I am not convinced that pg_dump is the only client-side code
that would get broken, either.

>> Further other approach is to change the reloptions code so that it
>> always stores the plain value without the units (i.e., 1000 is stored
>> if 1s is specified in autovacuum_vacuum_cost_delay)in pg_class.

> This seems like the way to go.

Yeah, it's the best idea I can think of either. It's a tad annoying but
I think we don't want to take the compatibility risks of storing
unit-ified values in reloptions.

In the meantime, the buildfarm is still all red. Can we please revert
this patch until a fixed version is ready?

regards, tom lane

On Fri, Aug 29, 2014 at 4:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Aug 28, 2014 at 12:22 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> Another approach is to change pg_dump so that it encloses the relopt
>>> values with single quotes. This is the same approach as what
>>> pg_dumpall does for database or role-specific settings.
>
>> To me, this doesn't seem nearly important enough to justify breaking
>> pg_dump compatibility. AAUI, this is just a cosmetic improvement, so
>> we shouldn't break functional things for that.
>
> Indeed. I am not convinced that pg_dump is the only client-side code
> that would get broken, either.
>
>>> Further other approach is to change the reloptions code so that it
>>> always stores the plain value without the units (i.e., 1000 is stored
>>> if 1s is specified in autovacuum_vacuum_cost_delay)in pg_class.
>
>> This seems like the way to go.

Agreed.

>
> Yeah, it's the best idea I can think of either. It's a tad annoying but
> I think we don't want to take the compatibility risks of storing
> unit-ified values in reloptions.
>
> In the meantime, the buildfarm is still all red. Can we please revert
> this patch until a fixed version is ready?

OK, reverted.

Regards,

--
Fujii Masao

On Thu, Aug 28, 2014 at 3:27 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> OK, reverted.
As this patch needs more care in the way it is stored in pg_class as
reloptions (need more logic to parse correctly units from the actual
values that are store), I just marked it as returned with feedback.
--
Michael