[RFC] Extend namespace of valid guc names

Hello

Why? There are no multilevels structures in pg. Variables should be joined
with schemas or extensions. 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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�

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers