Обсуждение: [RFC] Extend namespace of valid guc names
Hi, Currently guc-file.c allows the following as guc names: ID {LETTER}{LETTER_OR_DIGIT}* QUALIFIED_ID {ID}"."{ID} That is either one token starting with a letter followed by numbers or letters or exactly two of those separated by a dot. Those restrictions are existing for neither SET/set_config() via SQL nor for postgres -c styles of setting options. I propose loosening those restrictions to a) allow repeatedly qualified names like a.b.c b) allow variables to start with a digit from the second level onwards. Additionally, should we perhaps enforce the same rules for -c and set_config()/SET? Trivial patch that only extends the space of valid names for postgresql.conf attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
<p dir="ltr">Hello<p dir="ltr">Why? There are no multilevels structures in pg. Variables should be joined with schemas orextensions. Other levels are messy.
On 2013-02-25 22:27:21 +0100, Pavel Stehule wrote: > Why? The concrete usecase is having some form of nesting available for a shared_library. shared_preload_libraries = 'bdr' bdr.connections = 'a, b' bdr.a.dsn = '...' bdr.a.xxx = '...' bdr.b.dsn = '...' > There are no multilevels structures in pg. Variables should be joined > with schemas or extensions. Other levels are messy. So? What does the one existing level SQL have to do with postgresql.conf? And why is it messy? Not that as I said before SET foo.bar.blub = '1'; currently is already allowed. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 02/25/2013 04:34 PM, Andres Freund wrote: > Not that as I said before > SET foo.bar.blub = '1'; currently is already allowed. I gather you mean "Note". I agree that it seems very odd to allow this in one context and forbid it in another. cheers andrew
Andres Freund <andres@2ndquadrant.com> writes: > I propose loosening those restrictions to > a) allow repeatedly qualified names like a.b.c If SET allows it, I guess we can allow it here. But is a grammar change really all that is needed to make it work from the file? > b) allow variables to start with a digit from the second level onwards. That seems like a seriously bad idea. I note that SET does *not* allow this; furthermore it seems like a considerable weakening of our ability to detect silly typos in config files. Nor did you offer a use-case to justify it. regards, tom lane
On 2013-02-25 21:13:25 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > I propose loosening those restrictions to > > a) allow repeatedly qualified names like a.b.c > > If SET allows it, I guess we can allow it here. But is a grammar change > really all that is needed to make it work from the file? Seems so. There's no additional validation that I could find anywhere. And a simple test confirmed it works. postgres=# SHOW foo.bar.blub;foo.bar.blub --------------1 (1 row) Just for reference, thats the grammar for SET/SHOW: var_name: ColId { $$ = $1; } | var_name '.' ColId > > b) allow variables to start with a digit from the second level onwards. > > That seems like a seriously bad idea. I note that SET does *not* allow > this; furthermore it seems like a considerable weakening of our ability > to detect silly typos in config files. Nor did you offer a use-case > to justify it. The use-case I had in mind was bdr.1.dsn = ... bdr.2.dsn = ... bdr.3.dsn = ... bdr.4.dsn = ... which is what I had used via -c. But I guess it can easy enough be replaced by node_$i or something. Any arguments whether we should try to validate -c SET/SHOW, set_config() and -c the same? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-02-26 13:08:54 +0100, Andres Freund wrote: > Just for reference, thats the grammar for SET/SHOW: > > var_name: ColId { $$ = $1; } > | var_name '.' ColId > > Any arguments whether we should try to validate postgres -c, > set_config and SET/SHOW the same? Any input here? Currently set_config() and postgres -c basically don't have any restrictions on the passed guc name whereas SHOW, SET and postgresql.conf do. Greetings, Andres Freund --Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-02-25 21:13:25 -0500, Tom Lane wrote: > > b) allow variables to start with a digit from the second level onwards. > > That seems like a seriously bad idea. I note that SET does *not* allow > this; Hm. One thing about this is that we currently allow something silly as: SET "1"."1bar""blub" = 3; So I'd like to either restrict SET here or allow the same for guc-file.l parsed GUCs. Any opinions? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes: > On 2013-02-25 21:13:25 -0500, Tom Lane wrote: >>> b) allow variables to start with a digit from the second level onwards. >> That seems like a seriously bad idea. I note that SET does *not* allow >> this; > Hm. One thing about this is that we currently allow something silly as: > SET "1"."1bar""blub" = 3; > So I'd like to either restrict SET here or allow the same for guc-file.l > parsed GUCs. Any opinions? Well, if you feel an absolute compulsion to make them consistent, I'd go with making SET disallow creation of variables with names the file parser wouldn't recognize. But why is it such a bad thing if SET can do that? The whole reason we allow SET to create new variables at all is that the universe of things you can have as session-local values is larger than the set of parameters that are allowed in postgresql.conf. So I'm missing why we need such a restriction. regards, tom lane
On 2013-09-06 10:13:23 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > >> That seems like a seriously bad idea. I note that SET does *not* allow > >> this; > > > Hm. One thing about this is that we currently allow something silly as: > > SET "1"."1bar""blub" = 3; > > > So I'd like to either restrict SET here or allow the same for guc-file.l > > parsed GUCs. Any opinions? > > Well, if you feel an absolute compulsion to make them consistent, I'd > go with making SET disallow creation of variables with names the file > parser wouldn't recognize. But why is it such a bad thing if SET can > do that? The whole reason we allow SET to create new variables at all > is that the universe of things you can have as session-local values is > larger than the set of parameters that are allowed in postgresql.conf. > So I'm missing why we need such a restriction. Well, it's confusing for users, i.e. me. I've several times now prototyped stuff that was supposed to be configurable in postgresql.conf by either passing the options to postgres -c or by doing user level SETs. Only to then later discover that what I've prototyped doesn't work because the restrictions in postgresql.conf are way stricter. Also, ALTER SYSTEM SET is going to need a similar restriction as well, otherwise the server won't restart although the GUCs pass validation... Greetings, Andres Freund --Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes: > On 2013-09-06 10:13:23 -0400, Tom Lane wrote: >> Well, if you feel an absolute compulsion to make them consistent, I'd >> go with making SET disallow creation of variables with names the file >> parser wouldn't recognize. But why is it such a bad thing if SET can >> do that? > Also, ALTER SYSTEM SET is going to need a similar restriction as well, > otherwise the server won't restart although the GUCs pass validation... Well, sure, but I would think that ALTER SYSTEM SET should be constrained to only set known GUCs, not invent new ones on the fly. regards, tom lane
On 09/06/2013 08:48 PM, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: >> On 2013-09-06 10:13:23 -0400, Tom Lane wrote: >>> Well, if you feel an absolute compulsion to make them consistent, I'd >>> go with making SET disallow creation of variables with names the file >>> parser wouldn't recognize. But why is it such a bad thing if SET can >>> do that? >> Also, ALTER SYSTEM SET is going to need a similar restriction as well, >> otherwise the server won't restart although the GUCs pass validation... > Well, sure, but I would think that ALTER SYSTEM SET should be constrained > to only set known GUCs, not invent new ones on the fly. What's the reasoning behind this ? I was assuming that ALTER SYSTEM SET would allow all GUCs which do not require restart which includes all "newly invented" ones. Cheers -- Hannu Krosing PostgreSQL Consultant Performance, Scalability and High Availability 2ndQuadrant Nordic OÜ
Hannu Krosing <hannu@2ndQuadrant.com> writes: > On 09/06/2013 08:48 PM, Tom Lane wrote: >> Well, sure, but I would think that ALTER SYSTEM SET should be constrained >> to only set known GUCs, not invent new ones on the fly. > What's the reasoning behind this ? If you don't know what a GUC is, you don't know what are valid values for it, and thus you might write an illegal value into auto.conf (or whatever we're calling it this week). That could have consequences as bad as failure to restart, should the DBA decide to preload the module defining that GUC, which would then complain about the bad value during postmaster start. > I was assuming that ALTER SYSTEM SET would allow all GUCs which > do not require restart which includes all "newly invented" ones. I do not believe that the former need imply the latter, nor do I see a strong use-case for allowing ALTER SYSTEM SET on session-local GUCs, which is what any truly invented-on-the-fly GUCs would be. The whole business with session-local GUCs is pretty much a kluge anyway, which we might want to retire or redefine someday; so I'd much prefer that ALTER SYSTEM SET stayed out of it. regards, tom lane
On 2013-09-06 14:48:33 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On 2013-09-06 10:13:23 -0400, Tom Lane wrote: > >> Well, if you feel an absolute compulsion to make them consistent, I'd > >> go with making SET disallow creation of variables with names the file > >> parser wouldn't recognize. But why is it such a bad thing if SET can > >> do that? > > > Also, ALTER SYSTEM SET is going to need a similar restriction as well, > > otherwise the server won't restart although the GUCs pass validation... > > Well, sure, but I would think that ALTER SYSTEM SET should be constrained > to only set known GUCs, not invent new ones on the fly. Hm. That sounds inconvenient to me. Consider something like configuring the system to use auto_explain henceforth. ALTER SYSTEM SET shared_preload_libraries = 'auto_explain'; ALTER SYSTEM SET auto_explain.log_min_duration = 100; It seems weird to forbid doing that and requiring a manual LOAD when we don't do so for normal SETs. I can live with the restriction if we decide it's a good idea, I just wouldn't appreciate it. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Sep 6, 2013 at 6:31 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-09-06 14:48:33 -0400, Tom Lane wrote: >> Andres Freund <andres@2ndquadrant.com> writes: >> > On 2013-09-06 10:13:23 -0400, Tom Lane wrote: >> >> Well, if you feel an absolute compulsion to make them consistent, I'd >> >> go with making SET disallow creation of variables with names the file >> >> parser wouldn't recognize. But why is it such a bad thing if SET can >> >> do that? >> >> > Also, ALTER SYSTEM SET is going to need a similar restriction as well, >> > otherwise the server won't restart although the GUCs pass validation... >> >> Well, sure, but I would think that ALTER SYSTEM SET should be constrained >> to only set known GUCs, not invent new ones on the fly. > > Hm. That sounds inconvenient to me. Consider something like configuring > the system to use auto_explain henceforth. > ALTER SYSTEM SET shared_preload_libraries = 'auto_explain'; > ALTER SYSTEM SET auto_explain.log_min_duration = 100; > > It seems weird to forbid doing that and requiring a manual LOAD when we > don't do so for normal SETs. I can live with the restriction if we > decide it's a good idea, I just wouldn't appreciate it. I'm with Tom on this one: I think this will save more pain than it causes. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Fri, Sep 6, 2013 at 6:31 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> On 2013-09-06 14:48:33 -0400, Tom Lane wrote: >>> Well, sure, but I would think that ALTER SYSTEM SET should be constrained >>> to only set known GUCs, not invent new ones on the fly. >> Hm. That sounds inconvenient to me. Consider something like configuring >> the system to use auto_explain henceforth. >> ALTER SYSTEM SET shared_preload_libraries = 'auto_explain'; >> ALTER SYSTEM SET auto_explain.log_min_duration = 100; > I'm with Tom on this one: I think this will save more pain than it causes. So far as that example goes, I'm not suggesting that "ALTER SYSTEM SET auto_explain.log_min_duration" should be forbidden altogether. I *am* saying that it should only be allowed when auto_explain is loaded in the current session, so that we can find out whether the proposed value is allowed by the module that defines the GUC. Of course, this is not completely bulletproof, since it will fail if the defining module changes its mind from time to time about what are valid values of the GUC :-(. But promising to restart in the face of that kind of inconsistency is hopeless. On the other hand, not checking at all is just asking for failures. regards, tom lane
On 2013-02-25 22:15:33 +0100, Andres Freund wrote: > Currently guc-file.c allows the following as guc names: > > ID {LETTER}{LETTER_OR_DIGIT}* > QUALIFIED_ID {ID}"."{ID} > > That is either one token starting with a letter followed by numbers or > letters or exactly two of those separated by a dot. > Those restrictions are existing for neither SET/set_config() via SQL nor > for postgres -c styles of setting options. > > I propose loosening those restrictions to > a) allow repeatedly qualified names like a.b.c > b) allow variables to start with a digit from the second level onwards. The attached patch does a) but not b) as Tom argued it's not allowed in a plain fashion in SQL either. There you need to quote the variable name. > Additionally, should we perhaps enforce the same rules for -c and > set_config()/SET? The discussion seems to have concluded that this isn't neccessary, so I haven't included this. The patch still is pretty trivial - I've looked around and I really didn't see anything else that requires changes. I haven't even found documentation to adapt. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
On Sat, Sep 14, 2013 at 5:21 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> a) allow repeatedly qualified names like a.b.c > > The attached patch does a) What is the use case for this change? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-09-17 10:23:30 -0400, Robert Haas wrote: > On Sat, Sep 14, 2013 at 5:21 PM, Andres Freund <andres@2ndquadrant.com> wrote: > >> a) allow repeatedly qualified names like a.b.c > > > > The attached patch does a) > > What is the use case for this change? Check http://archives.postgresql.org/message-id/20130225213400.GF3849%40awork2.anarazel.de or the commit message of the patch. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Sep 17, 2013 at 10:27 AM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-09-17 10:23:30 -0400, Robert Haas wrote: >> On Sat, Sep 14, 2013 at 5:21 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> >> a) allow repeatedly qualified names like a.b.c >> > >> > The attached patch does a) >> >> What is the use case for this change? > > Check http://archives.postgresql.org/message-id/20130225213400.GF3849%40awork2.anarazel.de > or the commit message of the patch. That's more or less what I figured. One thing I'm uncomfortable with is that, while this is useful for extensions, what do we do when we have a similar requirement for core? The proposed GUC name is of the format extension.user_defined_object_name.property_name; but omitting the extension name would lead to user_defined_object_name.property_name, which doesn't feel good as a method for naming core GUCs. The user defined object name might just so happen to be an extension name. More generally, it seems like this is trying to take structured data and fit into the GUC mechanism, and I'm not sure that's going to be a real good fit. I mean, pg_hba.conf could be handled this way. You could have hba.1.type = local, hba.1.database = all, hba.1.user = all, hba.2.type = host, etc. But I doubt that any of us would consider that an improvement over the actual approach of having a separate file with that information. If it's not a good fit for pg_hba.conf, why is it a good fit for anything else we want to do? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 2013-09-17 16:24:34 -0400, Robert Haas wrote: > On Tue, Sep 17, 2013 at 10:27 AM, Andres Freund <andres@2ndquadrant.com> wrote: > > On 2013-09-17 10:23:30 -0400, Robert Haas wrote: > >> What is the use case for this change? > > > > Check http://archives.postgresql.org/message-id/20130225213400.GF3849%40awork2.anarazel.de > > or the commit message of the patch. > > That's more or less what I figured. One thing I'm uncomfortable with > is that, while this is useful for extensions, what do we do when we > have a similar requirement for core? The proposed GUC name is of the > format extension.user_defined_object_name.property_name; but omitting > the extension name would lead to > user_defined_object_name.property_name, which doesn't feel good as a > method for naming core GUCs. The user defined object name might just > so happen to be an extension name. I think that ship has long since sailed. postgresql.conf has allowed foo.bar style GUCs via custom_variable_classes for a long time, and these days we don't even require that but allow them generally. Also, SET, postgres -c, and SELECT set_config() already don't have the restriction to one dot in the variable name. I think if we're going to use something like that for postgres, we'll have to live with the chance of breaking applications (although I don't think it's that big for most of the variables where I can forsee the use). > If it's not a good fit for pg_hba.conf, why is it a good fit for > anything else we want to do? Well, for now it's primarily there for extensions, not for core code. Although somebody has already asked when I'd submit the patch because they could use it for their patch... I don't really find the pg_hba.conf comparison terribly convincing. As an extension, how would you prefer to formulate the configuration in the example? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Sep 18, 2013 at 4:26 AM, Andres Freund <andres@2ndquadrant.com> wrote: > Hi, > > On 2013-09-17 16:24:34 -0400, Robert Haas wrote: >> On Tue, Sep 17, 2013 at 10:27 AM, Andres Freund <andres@2ndquadrant.com> wrote: >> > On 2013-09-17 10:23:30 -0400, Robert Haas wrote: >> >> What is the use case for this change? >> > >> > Check http://archives.postgresql.org/message-id/20130225213400.GF3849%40awork2.anarazel.de >> > or the commit message of the patch. >> >> That's more or less what I figured. One thing I'm uncomfortable with >> is that, while this is useful for extensions, what do we do when we >> have a similar requirement for core? The proposed GUC name is of the >> format extension.user_defined_object_name.property_name; but omitting >> the extension name would lead to >> user_defined_object_name.property_name, which doesn't feel good as a >> method for naming core GUCs. The user defined object name might just >> so happen to be an extension name. > > I think that ship has long since sailed. postgresql.conf has allowed > foo.bar style GUCs via custom_variable_classes for a long time, and > these days we don't even require that but allow them generally. Also, > SET, postgres -c, and SELECT set_config() already don't have the > restriction to one dot in the variable name. It's even explained in document that a two-part name is allowed for Customized Options at link: http://www.postgresql.org/docs/devel/static/runtime-config-custom.html > I think if we're going to use something like that for postgres, we'll > have to live with the chance of breaking applications (although I don't > think it's that big for most of the variables where I can forsee the use). > >> If it's not a good fit for pg_hba.conf, why is it a good fit for >> anything else we want to do? > > Well, for now it's primarily there for extensions, not for core > code. Although somebody has already asked when I'd submit the patch > because they could use it for their patch... I had reviewed and tested the patch. The purpose of patch is to allow more than two-part parameter name through postgresql.conf. I think it's useful to allow more than two-parameter names mainly because a) in database terminology, people are quite use to think for more than two-part names, for ex. any object <database_name>.<schema_name>.<object_name> here object_name can be table_name,index_name, etc. b) it already works in SET/set_config(). The code is fine and I had done tests for various parameters (more than two-part) in postgresql.conf all of which works fine (Select pg_reload_conf() and show of parameter name). For example: foo2.bar2.bar1 = 3 foo2.bar2.bar1 = 4 foo2.bar2.bar1.bar1 = 4 a.b._c = 4 d.e.f =on d.e.f.g =true d.e.f.g.h = 'abcdef' a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.y.z.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z.a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z = 4 Apart from this, quite a few negative tests (setting invalid names) also works fine (select pg_reload_conf() shows error on server). We can modify the documentation for Customized Options to mention about support for more than two-part name. On a side note, although it doesn't have any relation with your patch, I found some minor problems in setting of configuration during test of this patch, so I am mentioning it here. I will look into these in detail later: Test-1 postgres=# select set_config('a.b.1.c','c',false);set_config ------------c (1 row) postgres=# show a.b.1.c; ERROR: syntax error at or near ".1" LINE 1: show a.b.1.c; allows to set variable name which has integer, but could not see it via show as other variables can be seen. Test-2 test to see if longer length string are compatible in all interfaces. set fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo.bar=1; Behavior of set command ------------------------- postgres=# set foooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo ooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo.bar=1; NOTICE: identifier "foooooooooooooooooooooooooooooooooooooooooooooooooooooooooo ooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo" will be tru ncated to "foooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo" SET postgres=# show foooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo. bar;foooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo -----------------------------------------------------------------1 (1 row) Behavior when the same is set though postgresql.conf ---------------------------------------------------- postgres=# show fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo.bar; NOTICE: identifier "foooooooooooooooooooooooooooooooooooooooooooooooooooooooooo ooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo" will be tru ncated to "foooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo" ERROR: unrecognized configuration parameter "fooooooooooooooooooooooooooooooooo ooooooooooooooooooooooooooooo.bar" postgres=# Although variable is loaded but you cannot see it. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On 2013-09-18 11:55:24 +0530, Amit Kapila wrote: > > I think that ship has long since sailed. postgresql.conf has allowed > > foo.bar style GUCs via custom_variable_classes for a long time, and > > these days we don't even require that but allow them generally. Also, > > SET, postgres -c, and SELECT set_config() already don't have the > > restriction to one dot in the variable name. > > It's even explained in document that a two-part name is allowed for > Customized Options at link: > http://www.postgresql.org/docs/devel/static/runtime-config-custom.html Oh I somehow missed that. I'll need to patch that as well. > Apart from this, quite a few negative tests (setting invalid names) > also works fine (select pg_reload_conf() shows error on server). > On a side note, although it doesn't have any relation with your patch, > I found some minor problems in setting of configuration during test of > this patch, so I am mentioning it here. I will look into these in > detail later: Most of those have been discussed in another subthread. While I still am not sure that I agree, the concensus was that we shouldn't do anything about that for now. > > Test-1 > postgres=# select set_config('a.b.1.c','c',false); > set_config > ------------ > c > (1 row) > > postgres=# show a.b.1.c; > ERROR: syntax error at or near ".1" > LINE 1: show a.b.1.c; You can show it with a.b."1".c, but those can't be set via guc-file.l. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-09-18 11:02:50 +0200, Andres Freund wrote: > On 2013-09-18 11:55:24 +0530, Amit Kapila wrote: > > > > I think that ship has long since sailed. postgresql.conf has allowed > > > foo.bar style GUCs via custom_variable_classes for a long time, and > > > these days we don't even require that but allow them generally. Also, > > > SET, postgres -c, and SELECT set_config() already don't have the > > > restriction to one dot in the variable name. > > > > It's even explained in document that a two-part name is allowed for > > Customized Options at link: > > http://www.postgresql.org/docs/devel/static/runtime-config-custom.html > > Oh I somehow missed that. I'll need to patch that as well. Updated patch attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Вложения
On Tue, Sep 17, 2013 at 6:56 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> That's more or less what I figured. One thing I'm uncomfortable with >> is that, while this is useful for extensions, what do we do when we >> have a similar requirement for core? The proposed GUC name is of the >> format extension.user_defined_object_name.property_name; but omitting >> the extension name would lead to >> user_defined_object_name.property_name, which doesn't feel good as a >> method for naming core GUCs. The user defined object name might just >> so happen to be an extension name. > > I think that ship has long since sailed. postgresql.conf has allowed > foo.bar style GUCs via custom_variable_classes for a long time, and > these days we don't even require that but allow them generally. Also, > SET, postgres -c, and SELECT set_config() already don't have the > restriction to one dot in the variable name. Well, we should make it consistent, but I'm not sure that settles the question of which direction to go. >> If it's not a good fit for pg_hba.conf, why is it a good fit for >> anything else we want to do? > > Well, for now it's primarily there for extensions, not for core > code. Although somebody has already asked when I'd submit the patch > because they could use it for their patch... > I don't really find the pg_hba.conf comparison terribly convincing. As > an extension, how would you prefer to formulate the configuration > in the example? Well, to me it looks like what you've got there is a table. And I don't have a clear feeling of how that ought to be represented in configuration-land, but trying to jam it into the GUC machinery seems awkward at best. I think we need a way of handling tabular configuration data, but I'm not convinced this is it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-09-19 14:57:53 -0400, Robert Haas wrote: > On Tue, Sep 17, 2013 at 6:56 PM, Andres Freund <andres@2ndquadrant.com> wrote: > > I think that ship has long since sailed. postgresql.conf has allowed > > foo.bar style GUCs via custom_variable_classes for a long time, and > > these days we don't even require that but allow them generally. Also, > > SET, postgres -c, and SELECT set_config() already don't have the > > restriction to one dot in the variable name. > > Well, we should make it consistent, but I'm not sure that settles the > question of which direction to go. I suggested making it consistent in either form elsewhere in this thread, Tom wasn't convinced. I think because of backward compatibility concerns we hardly can restrict the valid namespace in all forms to the most restrictive form (i.e. guc-file.l). Quite some people use GUCs as variables. Maybe we can forbid the more absurd variants (SET "..."."..." = 3; SELECT set_config('...', '1', true)), but restricting it entirely seems to cause pain for no reason. > >> If it's not a good fit for pg_hba.conf, why is it a good fit for > >> anything else we want to do? > > > > Well, for now it's primarily there for extensions, not for core > > code. Although somebody has already asked when I'd submit the patch > > because they could use it for their patch... > > I don't really find the pg_hba.conf comparison terribly convincing. As > > an extension, how would you prefer to formulate the configuration > > in the example? > > Well, to me it looks like what you've got there is a table. And I > don't have a clear feeling of how that ought to be represented in > configuration-land, but trying to jam it into the GUC machinery seems > awkward at best. I think we need a way of handling tabular > configuration data, but I'm not convinced this is it. Well, it has two major advantages from my pov: a) we already have it from SQL and people already use it that way b) it's dirt simple and it doesn't allow anything not allowed before via other means. Maybe you can invent something nicer to look at that's also parsed before the server starts, but I find it hard to see the need TBH. Especially as you will want most of the infrastructure GUCs provide... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Sep 19, 2013 at 2:48 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-09-18 11:02:50 +0200, Andres Freund wrote: >> On 2013-09-18 11:55:24 +0530, Amit Kapila wrote: >> >> > > I think that ship has long since sailed. postgresql.conf has allowed >> > > foo.bar style GUCs via custom_variable_classes for a long time, and >> > > these days we don't even require that but allow them generally. Also, >> > > SET, postgres -c, and SELECT set_config() already don't have the >> > > restriction to one dot in the variable name. >> > >> > It's even explained in document that a two-part name is allowed for >> > Customized Options at link: >> > http://www.postgresql.org/docs/devel/static/runtime-config-custom.html >> >> Oh I somehow missed that. I'll need to patch that as well. > > Updated patch attached. old part of line - PostgreSQL will accept a setting for any two-part parameter name. new part of line + PostgreSQL will accept a setting for any parameter name containing at least one dot. If I read new part of line just in context of custom options, it makes sense, but when I compare with old line I think below lines or variant of them might make more sense as this line is not restricted to just custom options: a. PostgreSQL will accept a setting for any parameter name containing two or more part parameter names. b. PostgreSQL will accept a setting for any parameter name containing one or more dots in parts of parameter names. It's just how user interpret any line, so may be your line is more meaningful to some users. If you don't think there is any need to change then keep as it is and let committer decide about it. I don't have any big problem with the current wording. I think Robert is still not fully convinced about this patch, but from my side review of patch with the current scope is complete, so I will mark it as Ready For Committer if nobody has objections for the same. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Thu, Sep 19, 2013 at 6:19 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-09-19 14:57:53 -0400, Robert Haas wrote: >> On Tue, Sep 17, 2013 at 6:56 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> > I think that ship has long since sailed. postgresql.conf has allowed >> > foo.bar style GUCs via custom_variable_classes for a long time, and >> > these days we don't even require that but allow them generally. Also, >> > SET, postgres -c, and SELECT set_config() already don't have the >> > restriction to one dot in the variable name. >> >> Well, we should make it consistent, but I'm not sure that settles the >> question of which direction to go. > > I suggested making it consistent in either form elsewhere in this > thread, Tom wasn't convinced. I think because of backward compatibility > concerns we hardly can restrict the valid namespace in all forms to the > most restrictive form (i.e. guc-file.l). Quite some people use GUCs as > variables. > Maybe we can forbid the more absurd variants (SET "..."."..." = 3; > SELECT set_config('...', '1', true)), but restricting it entirely seems > to cause pain for no reason. Yeah, I don't think I understand Tom's reasoning. I mean, why shouldn't there be ONE rule for what can be a GUC? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Sep 20, 2013 at 9:48 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Sep 19, 2013 at 2:48 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> On 2013-09-18 11:02:50 +0200, Andres Freund wrote: >>> On 2013-09-18 11:55:24 +0530, Amit Kapila wrote: >>> >>> > > I think that ship has long since sailed. postgresql.conf has allowed >>> > > foo.bar style GUCs via custom_variable_classes for a long time, and >>> > > these days we don't even require that but allow them generally. Also, >>> > > SET, postgres -c, and SELECT set_config() already don't have the >>> > > restriction to one dot in the variable name. >>> > >>> > It's even explained in document that a two-part name is allowed for >>> > Customized Options at link: >>> > http://www.postgresql.org/docs/devel/static/runtime-config-custom.html >>> >>> Oh I somehow missed that. I'll need to patch that as well. >> >> Updated patch attached. > > old part of line > - PostgreSQL will accept a setting for any two-part parameter name. > > new part of line > + PostgreSQL will accept a setting for any parameter name containing > at least one dot. > > If I read new part of line just in context of custom options, it makes > sense, but when I compare with old line I think below lines or variant > of them might make more sense as this line is not restricted to just > custom options: > > a. PostgreSQL will accept a setting for any parameter name containing > two or more part parameter names. > b. PostgreSQL will accept a setting for any parameter name containing > one or more dots in parts of parameter names. > > It's just how user interpret any line, so may be your line is more > meaningful to some users. If you don't think there is any need to > change then keep as it is and let committer decide about it. I don't > have any big problem with the current wording. > > I think Robert is still not fully convinced about this patch, but from > my side review of patch with the current scope is complete, so I will > mark it as Ready For Committer if nobody has objections for the same. I had marked this patch as Ready For Committer, as I think in it's current scope definition it's ready for next level of review. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
I think the idea that we should consider a different way of handling tabular configuration data has merit. In fact, how much sense does it make to have these options (the ones for which this patch is being written) be GUCs in the first place? ALTER USER/DATABASE don't work for them, they can't be usefully changed in the commandline, there's no working SET. If we had some way to plug these into pg_hba.conf parsing machinery (which is tabular data), I would suggest that. But that doesn't sound really sensible. I think the idea of putting this configuratio data in a separate file, and perhaps a more convenient format than three-level GUC options, should be considered. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Sep 23, 2013 at 7:06 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > I think the idea that we should consider a different way of handling > tabular configuration data has merit. In fact, how much sense does it > make to have these options (the ones for which this patch is being > written) be GUCs in the first place? This is mainly for Customized option and or that it depends on user, how he wants to name the variables. > ALTER USER/DATABASE don't work for > them, they can't be usefully changed in the commandline, there's no > working SET. Do you mean to say that it doesn't work for SET command? As it is discussed upthread that if it works for set_config()/SET,.. so it is better that it should be allowed through postgresql.conf > If we had some way to plug these into pg_hba.conf parsing machinery > (which is tabular data), I would suggest that. But that doesn't sound > really sensible. I think the idea of putting this configuratio data > in a separate file, and perhaps a more convenient format than > three-level GUC options, should be considered. This will be really better if we can figure out a more sophisticated way to handle, but I think if it currently works with other mechanism's of GUC settings (set_config,SET, etc.), then what is the harm in allowing it through postgresql.conf file? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Mon, Sep 23, 2013 at 9:36 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > I think the idea that we should consider a different way of handling > tabular configuration data has merit. In fact, how much sense does it > make to have these options (the ones for which this patch is being > written) be GUCs in the first place? ALTER USER/DATABASE don't work for > them, they can't be usefully changed in the commandline, there's no > working SET. > > If we had some way to plug these into pg_hba.conf parsing machinery > (which is tabular data), I would suggest that. But that doesn't sound > really sensible. I think the idea of putting this configuratio data > in a separate file, and perhaps a more convenient format than > three-level GUC options, should be considered. All very good points, IMHO. In a lot of cases, what you want is sneazle.list='foo,bar' sneazle.foo.prop1='zatz' sneazle.bar.prop1='frotz' etc. But that means that the set of GUCs that exist to be SET needs to change every time sneazle.list changes, and we haven't got any good mechanism for that. I really think we're trying to squeeze a square peg into a round hole here, and I accordingly propose to mark this patch rejected. It seems to me that if an extension wants to read and parse a configuration file in $PGDATA, it doesn't need any special core support for that. If there's enough consistency in terms of what those configuration files look like across various extensions, we might choose to provide a set of common tools in core to help parse them. But I'm not too convinced any useful pattern will emerge. Another option is to store the data in an actual table. One could have sneazle.configtable='dbname.schemaname.tablename', for example. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi Robert, On 2013-10-03 14:39:46 -0400, Robert Haas wrote: > On Mon, Sep 23, 2013 at 9:36 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > I think the idea that we should consider a different way of handling > > tabular configuration data has merit. In fact, how much sense does it > > make to have these options (the ones for which this patch is being > > written) be GUCs in the first place? ALTER USER/DATABASE don't work for > > them, they can't be usefully changed in the commandline, there's no > > working SET. > > > > If we had some way to plug these into pg_hba.conf parsing machinery > > (which is tabular data), I would suggest that. But that doesn't sound > > really sensible. I think the idea of putting this configuratio data > > in a separate file, and perhaps a more convenient format than > > three-level GUC options, should be considered. > > All very good points, IMHO. In a lot of cases, what you want is > > sneazle.list='foo,bar' > sneazle.foo.prop1='zatz' > sneazle.bar.prop1='frotz' > etc. > > But that means that the set of GUCs that exist to be SET needs to > change every time sneazle.list changes, and we haven't got any good > mechanism for that. It'd be pretty easy to have a function that removes everything inside a certain namespace. That'd be enough to make EmitWarningsOnPlaceholders() work, right? > I really think we're trying to squeeze a square > peg into a round hole here, and I accordingly propose to mark this > patch rejected. > It seems to me that if an extension wants to read and parse a > configuration file in $PGDATA, it doesn't need any special core > support for that. If there's enough consistency in terms of what > those configuration files look like across various extensions, we > might choose to provide a set of common tools in core to help parse > them. But I'm not too convinced any useful pattern will emerge. The problem is that such configuration formats needs to deal with a host of already solved things like: * triggering reload across backends * presentation layer: SHOW/pg_settings * The ability to override settings on certain levels: SET/ALTER DATABASE/ALTER ... * Correct handling of invalid values on reload and such * parsed early enough to allocate shared memory That's quite the truckload of things that need to be done by any new format. I don't really understand the resistance to the patch. It's a two line change that doesn't allow anything that wasn't already allowed by other means (SET, SELECT set_config(), -c). It sure isn't perfect for everything but for some scenarios it improves the scenario sufficiently that it'd make at least one extension author happy ;) > Another option is to store the data in an actual table. One could > have sneazle.configtable='dbname.schemaname.tablename', for example. Doesn't really work if your extension needs to do stuff during startup tho. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Oct 4, 2013 at 6:06 AM, Andres Freund <andres@2ndquadrant.com> wrote: > It'd be pretty easy to have a function that removes everything inside a > certain namespace. That'd be enough to make EmitWarningsOnPlaceholders() > work, right? Maybe, but I don't think you're going to paper over the problem that easily. The GUC mechanism just isn't decided to support settings that pop into and out of existence like that. It's not a coincidence that there's no UNSET commands for GUCs. We have RESET but that means "go back to the default", not "go away". You're trying to bend the mechanism to do something that it fundamentally wasn't designed to do.I don't think that's the right way to go, but if wedo decide to go in that direction it's going to take more than a trivial patch to get there. > The problem is that such configuration formats needs to deal with a host > of already solved things like: > * triggering reload across backends > * presentation layer: SHOW/pg_settings > * The ability to override settings on certain levels: SET/ALTER > DATABASE/ALTER ... > * Correct handling of invalid values on reload and such > * parsed early enough to allocate shared memory > > That's quite the truckload of things that need to be done by any new > format. True. > I don't really understand the resistance to the patch. It's a two line > change that doesn't allow anything that wasn't already allowed by other > means (SET, SELECT set_config(), -c). It sure isn't perfect for > everything but for some scenarios it improves the scenario sufficiently > that it'd make at least one extension author happy ;) That's true, but I think the fact that those things work is an oversight rather than a deliberate design decision. >> Another option is to store the data in an actual table. One could >> have sneazle.configtable='dbname.schemaname.tablename', for example. > > Doesn't really work if your extension needs to do stuff during startup > tho. Granted. There's not a perfect mechanism here. But I think we should be devoting some thought to what a good mechanism that could be used by core *and* contrib would look like, rather than thinking that a quick hack is going to make the pain go away. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-10-04 09:57:41 -0400, Robert Haas wrote: > On Fri, Oct 4, 2013 at 6:06 AM, Andres Freund <andres@2ndquadrant.com> wrote: > > It'd be pretty easy to have a function that removes everything inside a > > certain namespace. That'd be enough to make EmitWarningsOnPlaceholders() > > work, right? > > Maybe, but I don't think you're going to paper over the problem that > easily. The GUC mechanism just isn't decided to support settings that > pop into and out of existence like that. It's not a coincidence that > there's no UNSET commands for GUCs. We have RESET but that means "go > back to the default", not "go away". You're trying to bend the > mechanism to do something that it fundamentally wasn't designed to do. > I don't think that's the right way to go, but if we do decide to go > in that direction it's going to take more than a trivial patch to get > there. But that's not a new problem? It already exists and isn't really excerbated by this. > > I don't really understand the resistance to the patch. It's a two line > > change that doesn't allow anything that wasn't already allowed by other > > means (SET, SELECT set_config(), -c). It sure isn't perfect for > > everything but for some scenarios it improves the scenario sufficiently > > that it'd make at least one extension author happy ;) > > That's true, but I think the fact that those things work is an > oversight rather than a deliberate design decision. Yes, but it's already being used, so, while some minor restrictions probably aren't to problematic, forbidding multiple dots outright seems like unnecessarily breaking user applications. > >> Another option is to store the data in an actual table. One could > >> have sneazle.configtable='dbname.schemaname.tablename', for example. > > > > Doesn't really work if your extension needs to do stuff during startup > > tho. > > Granted. There's not a perfect mechanism here. But I think we should > be devoting some thought to what a good mechanism that could be used > by core *and* contrib would look like, rather than thinking that a > quick hack is going to make the pain go away. I agree that we could use some more infrastructure around configuration, but I fail to understand why it's this patch's duty to deliver it. And I don't see why this patch would endanger any more groundbreaking improvements. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Oct 4, 2013 at 10:14 AM, Andres Freund <andres@2ndquadrant.com> wrote: > But that's not a new problem? It already exists and isn't really > excerbated by this. ... > I agree that we could use some more infrastructure around configuration, > but I fail to understand why it's this patch's duty to deliver it. And I > don't see why this patch would endanger any more groundbreaking > improvements. You keep saying the ship has already sailed, but I think that's inaccurate. IMHO, we haven't committed to anything in this area as a matter of policy; I think the lack of a policy is demonstrated by the inconsistencies you point out. Now, if we are already committed to a policy of supporting the use case you're targeting with this patch, then you're right: this is just a trivial bug fix, and we ought to just take it for what it is and fix whatever other issues come up later. But if we're not committed to such a policy, then "support multi-level GUCs" is a new feature, and it's entirely right to think that, just like any other new feature, it needs to implement that feature completely rather than piecemeal. We know from experience that when certain features (e.g. hash indexes) are implemented incompletely, the resulting warts can remain behind more or less indefinitely. As I read the thread, Amit Kapila is in favor of your proposal. Pavel Stehule, Alvaro Herrera, and I all questioned whether multi-level GUCs were a good idea; at least 2 out of the 3 of us favor not committing the patch out of uncertainty that we wish to be on the hook to support such usages. Andrew Dunstan and Tom Lane agreed that the current behavior was inconsistent but neither clearly endorsed relaxing the checks in guc-file.l; in fact, Tom suggested tightening up SET instead. Not one person argued that multi-level GUCs were already a supported feature and that this patch was just plugging a gap in that feature; the documentation also disagrees with that interpretation. So I just don't think we have consensus that this is already the policy or that it is a policy we should adopt. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-10-04 11:04:53 -0400, Robert Haas wrote: > On Fri, Oct 4, 2013 at 10:14 AM, Andres Freund <andres@2ndquadrant.com> wrote: > > But that's not a new problem? It already exists and isn't really > > excerbated by this. > ... > > I agree that we could use some more infrastructure around configuration, > > but I fail to understand why it's this patch's duty to deliver it. And I > > don't see why this patch would endanger any more groundbreaking > > improvements. > > You keep saying the ship has already sailed, but I think that's > inaccurate. IMHO, we haven't committed to anything in this area as a > matter of policy; I think the lack of a policy is demonstrated by the > inconsistencies you point out. Well, it is SQL exposed behaviour that exists that way since the initial introduction of custom_variable_classes in 2004. Even if allowing multiple levels wasn't originally intended, ISTM that thats a long time to now declare it as a parsing bug. Especially as it doesn't actually hurt. > Now, if we are already committed to a policy of supporting the use > case you're targeting with this patch, then you're right: this is just > a trivial bug fix, and we ought to just take it for what it is and fix > whatever other issues come up later. But if we're not committed to > such a policy, then "support multi-level GUCs" is a new feature, and > it's entirely right to think that, just like any other new feature, it > needs to implement that feature completely rather than piecemeal. We > know from experience that when certain features (e.g. hash indexes) > are implemented incompletely, the resulting warts can remain behind > more or less indefinitely. Well, currently we're not committed to supporting it in postgresql.conf, but I think there's little chance of removing it from SQL. So not allowing it in the former doesn't buy us anything. > As I read the thread, Amit Kapila is in favor of your proposal. Pavel > Stehule, Alvaro Herrera, and I all questioned whether multi-level GUCs > were a good idea; at least 2 out of the 3 of us favor not committing > the patch out of uncertainty that we wish to be on the hook to support > such usages. Andrew Dunstan and Tom Lane agreed that the current > behavior was inconsistent but neither clearly endorsed relaxing the > checks in guc-file.l; in fact, Tom suggested tightening up SET > instead. That's slightly different than how I read it ;). Tom seems to argue against restricting SET further (28392.1378476803@sss.pgh.pa.us). But yes, I certainly haven't convinced everyone. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services