Thread: 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@2ndquadrant.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.fujii@gmail.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.paquier@gmail.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
Attachment
Michael Paquier <michael.paquier@gmail.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@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.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.fujii@gmail.com> wrote: > On Thu, Aug 28, 2014 at 11:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Michael Paquier <michael.paquier@gmail.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@gmail.com> writes: > On Thu, Aug 28, 2014 at 12:22 PM, Fujii Masao <masao.fujii@gmail.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Aug 28, 2014 at 12:22 PM, Fujii Masao <masao.fujii@gmail.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.fujii@gmail.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