TODO item: Allow more complex user/database default GUC settings

Alvaro Herrera wrote:

Implementation-side, it requires a new catalog (pg_settings), with the
following columns:

So, I've come up with the attached patch. It does not have the new
command yet, so you can do ALTER USER and ALTER DATABASE and it works,
but there's no way to set user-and-database-specific settings, short of
INSERT into the catalog. Apart from that it works nicely.

I'm just posting in case somebody has thoughts on the UI part of it.

Other things that need fixed:

- need to figure out locking for roles; this stuff must be synchronized
with role drop
- pg_shadow and pg_roles need a join to obtain settings
- two regression tests need their expected file updated
- catalog version bump

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

--On 25. August 2009 22:17:38 -0400 Alvaro Herrera
<alvherre@commandprompt.com> wrote:

I'm just posting in case somebody has thoughts on the UI part of it.

Other things that need fixed:

- need to figure out locking for roles; this stuff must be synchronized
with role drop
- pg_shadow and pg_roles need a join to obtain settings
- two regression tests need their expected file updated
- catalog version bump

Here's a first shot on this for my current review round. Patch needed to
re-merged into current CVS HEAD due to some merge conflicts, i've also
adjusted the regression tests (minor). On a first look, i like the patch
(especially the code for the utility commands accessing the settings is
better modularized now, which looks much nicer).

--
Thanks

Bernd

On Fri, Sep 18, 2009 at 4:03 PM, Bernd Helmle <mailings@oopsware.de> wrote:

--On 25. August 2009 22:17:38 -0400 Alvaro Herrera
<alvherre@commandprompt.com> wrote:

I'm just posting in case somebody has thoughts on the UI part of it.

Other things that need fixed:

- need to figure out locking for roles; this stuff must be synchronized
 with role drop
- pg_shadow and pg_roles need a join to obtain settings
- two regression tests need their expected file updated
- catalog version bump

Here's a first shot on this for my current review round. Patch needed to
re-merged into current CVS HEAD due to some merge conflicts, i've also
adjusted the regression tests (minor). On a first look, i like the patch
(especially the code for the utility commands accessing the settings is
better modularized now, which looks much nicer).

So is this ready to commit, or what?

...Robert

Robert Haas escribi�:

Here's a first shot on this for my current review round. Patch needed to
re-merged into current CVS HEAD due to some merge conflicts, i've also
adjusted the regression tests (minor). On a first look, i like the patch
(especially the code for the utility commands accessing the settings is
better modularized now, which looks much nicer).

So is this ready to commit, or what?

Not really :-( It needs some minor tweaks to qualify as a cleanup
patch, and further extra coding for there to be an actual new feature.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

--On 20. September 2009 22:56:53 -0400 Robert Haas <robertmhaas@gmail.com>
wrote:

So is this ready to commit, or what?

Not yet, see the comments Alvaro did upthread. Please note that i'm still
reviewing this one and i hope to post results tomorrow (there wasn't plenty
of free time over the weekend, i'm sorry).

--
Thanks

Bernd

On Mon, Sep 21, 2009 at 12:21 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Robert Haas escribió:

Here's a first shot on this for my current review round. Patch needed to
re-merged into current CVS HEAD due to some merge conflicts, i've also
adjusted the regression tests (minor). On a first look, i like the patch
(especially the code for the utility commands accessing the settings is
better modularized now, which looks much nicer).

So is this ready to commit, or what?

Not really :-(  It needs some minor tweaks to qualify as a cleanup
patch, and further extra coding for there to be an actual new feature.

So here's the followup question - do you intend to do one of those
things for this CommitFest, or should we mark this as Returned with
Feedback once Bernd posts the rest of his review?

...Robert

Robert Haas escribi�:

So here's the followup question - do you intend to do one of those
things for this CommitFest, or should we mark this as Returned with
Feedback once Bernd posts the rest of his review?

What feedback is it supposed to be returned with? Precisely what I
wanted is some feedback on the general idea. Brendan's "I like the
approach" is good, but is it enough to deter a later veto from someone
else?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera escribi�:

What feedback is it supposed to be returned with? Precisely what I
wanted is some feedback on the general idea. Brendan's "I like the
approach" is good, but is it enough to deter a later veto from someone
else?

s/Brendan/Bernd/

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

--On 21. September 2009 13:42:21 +0200 Bernd Helmle <mailings@oopsware.de>
wrote:

--On 20. September 2009 22:56:53 -0400 Robert Haas
<robertmhaas@gmail.com> wrote:

So is this ready to commit, or what?

Not yet, see the comments Alvaro did upthread. Please note that i'm still
reviewing this one and i hope to post results tomorrow (there wasn't
plenty of free time over the weekend, i'm sorry).

Here some further comments on the current patch:

- I'm not sure i like the name of the new system catalog pg_setting. Wie
already have pg_settings, i think this can be confusing. Maybe we need a
different name, e.g. pg_user_setting? This seems along the line with the
other *user* system objects (e.g. pg_stat_user_tables), where only "user
specific" objects are displayed.

- I have thought a little bit about the changes in the system views.
pg_roles and pg_shadow (as Alvaro already mentioned), need to be adjusted
(joined to the new catalog), to display rolconfig/useconfig. However, it's
unclear *how* to expose those information, for example, do we want to
expose roleconfig specific for the current database or for all databases
the role has a specific config for ?

- The code mentions the lack of lock synchronization. Maybe i'm missing
something obvious (its late here), but is there a reason this can't be done
by obtaining a lock on pg_authid (not sure about the backend user
initialization phase though) ?

- Regarding the missing UI: i go with Alvaro's proposal:

ALTER ROLE <rolename> [ALTER] DATABASE <dbname> SET <config> TO <value>;

Maybe we can make the 2nd ALTER optional.

Thoughts?

--
Thanks

Bernd

On Mon, Sep 21, 2009 at 12:23 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Robert Haas escribió:

So here's the followup question - do you intend to do one of those
things for this CommitFest, or should we mark this as Returned with
Feedback once Bernd posts the rest of his review?

What feedback is it supposed to be returned with?  Precisely what I
wanted is some feedback on the general idea.  Brendan's "I like the
approach" is good, but is it enough to deter a later veto from someone
else?

Well, you've hit there on one of the things that we don't always do
well. Many a patch author has posted an idea, received no feedback,
proceeded to implementation, and then the knives come out. On a good
day, the CommitFest process ensures that every patch gets a second
opinion, but it doesn't guarantee that a third opinion won't come
crawling out of the woodwork at a later date. In this respect, you're
actually operating at a slight advantage relative to most of us,
because you can post your revised patch and commit it if no one
objects too strongly, whereas I (for example) have to convince one of
about two people - Tom or Peter, for nearly anything I'm likely to
develop - to take an affirmative action on my behalf.

This whole phenomenon of proposals to which no objection was made at
the outset later getting flak for one reason or another is, I think, a
source of much frustration and discourages people from putting effort
into projects they might otherwise be willing to undertake. But I
haven't the least idea how to fix it, and I can't offer you any
guarantees with respect to the present situation either.

...Robert

On Tue, Sep 22, 2009 at 4:16 AM, Bernd Helmle <mailings@oopsware.de> wrote:

--On 21. September 2009 13:42:21 +0200 Bernd Helmle <mailings@oopsware.de>
wrote:

--On 20. September 2009 22:56:53 -0400 Robert Haas
<robertmhaas@gmail.com> wrote:

So is this ready to commit, or what?

Not yet, see the comments Alvaro did upthread. Please note that i'm still
reviewing this one and i hope to post results tomorrow (there wasn't
plenty of free time over the weekend, i'm sorry).

Here some further comments on the current patch:

- I'm not sure i like the name of the new system catalog pg_setting. Wie
already have pg_settings, i think this can be confusing. Maybe we need a
different name, e.g. pg_user_setting? This seems along the line with the
other *user* system objects (e.g. pg_stat_user_tables), where only "user
specific" objects are displayed.

- I have thought a little bit about the changes in the system views.
pg_roles and pg_shadow (as Alvaro already mentioned), need to be adjusted
(joined to the new catalog), to display rolconfig/useconfig. However, it's
unclear *how* to expose those information, for example, do we want to expose
roleconfig specific for the current database or for all databases the role
has a specific config for ?

- The code mentions the lack of lock synchronization. Maybe i'm missing
something obvious (its late here), but is there a reason this can't be done
by obtaining a lock on pg_authid (not sure about the backend user
initialization phase though) ?

- Regarding the missing UI: i go with Alvaro's proposal:

ALTER ROLE <rolename> [ALTER] DATABASE <dbname> SET <config> TO <value>;

Maybe we can make the 2nd ALTER optional.

Thoughts?

ON instead of second ALTER looks better, and IMHO DATABASE <dbname> should
be optional too:

ALTER ROLE <rolename> [ON DATABASE <dbname>] SET <config> TO <value>;

Best regards,
--
Lets call it Postgres

EnterpriseDB http://www.enterprisedb.com

gurjeet[.singh]@EnterpriseDB.com

singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
Twitter: singh_gurjeet
Skype: singh_gurjeet

Mail sent from my BlackLaptop device

Gurjeet Singh <singh.gurjeet@gmail.com> writes:

ON instead of second ALTER looks better, and IMHO DATABASE <dbname> should
be optional too:

ALTER ROLE <rolename> [ON DATABASE <dbname>] SET <config> TO <value>;

IN, not ON.

regards, tom lane

Alvaro Herrera <alvherre@commandprompt.com> writes:

Robert Haas escribi�:

So here's the followup question - do you intend to do one of those
things for this CommitFest, or should we mark this as Returned with
Feedback once Bernd posts the rest of his review?

What feedback is it supposed to be returned with? Precisely what I
wanted is some feedback on the general idea. Brendan's "I like the
approach" is good, but is it enough to deter a later veto from someone
else?

FWIW, I looked the patch over quickly, and I think it will be fine once
Bernd's comments are addressed. In particular I agree with the
objection to the name "pg_setting" as being confusingly close to
"pg_settings". But "pg_user_setting" isn't better. Maybe
"pg_db_role_settings"?

As far as the lock issue goes, I don't see any reason why the catalog
change creates a reason for new/different locking than we had before.
Any attempt to make concurrent updates to the same row will generate an
error, and that seems enough to me ...

regards, tom lane

--On 23. September 2009 14:10:39 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote:

FWIW, I looked the patch over quickly, and I think it will be fine once
Bernd's comments are addressed. In particular I agree with the
objection to the name "pg_setting" as being confusingly close to
"pg_settings". But "pg_user_setting" isn't better. Maybe
"pg_db_role_settings"?

Jepp, that's better, +1 from me.

I'm done with this, too, so i will mark this as "Returned with Feedback",
if no one objects?

--
Thanks

Bernd

On Wed, Sep 23, 2009 at 3:03 PM, Bernd Helmle <mailings@oopsware.de> wrote:

--On 23. September 2009 14:10:39 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote:

FWIW, I looked the patch over quickly, and I think it will be fine once
Bernd's comments are addressed.  In particular I agree with the
objection to the name "pg_setting" as being confusingly close to
"pg_settings".  But "pg_user_setting" isn't better.  Maybe
"pg_db_role_settings"?

Jepp, that's better, +1 from me.

I'm done with this, too, so i will mark this as "Returned with Feedback", if
no one objects?

It can be marked "Waiting on Author" if it's going to be reworked in
the next few days. If no plans to rework, or if the rework doesn't
materialize, then "Returned with Feedback".

...Robert

Tom Lane escribi�:

Gurjeet Singh <singh.gurjeet@gmail.com> writes:

ON instead of second ALTER looks better, and IMHO DATABASE <dbname> should
be optional too:

ALTER ROLE <rolename> [ON DATABASE <dbname>] SET <config> TO <value>;

IN, not ON.

This creates a parser conflict with
CREATE ROLE foo IN ROLE bar

I think it can be solved by splitting OptRoleElem in a set of
productions for ALTER and a superset of that for ALTER. I'll go try
that.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera escribi�:

I think it can be solved by splitting OptRoleElem in a set of
productions for ALTER and a superset of that for ALTER. I'll go try
that.

Right, that works. Updated patch attached; should solve the issues
raised in the thread. I renamed the catalog pg_db_role_setting as
suggested by Tom.

I have updated the pg_user and pg_roles definitions so that they include
the settings for the role, but only those that are not specific to any
database.

I have also added a view, whose only purpose is to convert the role and
database OIDs into names. It's been named pg_db_role_settings, but if
anyone has a better suggestion I'm all ears.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

Right, that works. Updated patch attached; should solve the issues
raised in the thread. I renamed the catalog pg_db_role_setting as
suggested by Tom.
...
I have also added a view, whose only purpose is to convert the role and
database OIDs into names. It's been named pg_db_role_settings, but if
anyone has a better suggestion I'm all ears.

I dislike the idea of having a catalog and a view whose names are the
same except for a plural. It's confusing as heck, because no one will
remember which is which.

Since pg_settings is the existing user view, I think pg_db_role_settings
is a reasonable choice for the new view, but then we need a different
name for the catalog. The only thing that comes to mind right now is
"pg_db_role_default", but I don't like it much. Anybody have other
suggestions?

regards, tom lane

On Sep 26, 2009, at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Right, that works. Updated patch attached; should solve the issues
raised in the thread. I renamed the catalog pg_db_role_setting as
suggested by Tom.
...
I have also added a view, whose only purpose is to convert the role
and
database OIDs into names. It's been named pg_db_role_settings, but
if
anyone has a better suggestion I'm all ears.

I dislike the idea of having a catalog and a view whose names are the
same except for a plural. It's confusing as heck, because no one will
remember which is which.

Since pg_settings is the existing user view, I think
pg_db_role_settings
is a reasonable choice for the new view, but then we need a different
name for the catalog. The only thing that comes to mind right now is
"pg_db_role_default", but I don't like it much. Anybody have other
suggestions?

The problem of having both a table and a closely related view is, IME,
one that comes up a lot. I think you just need to pick a convention
and stick with it. Mine is to append "_view" to the table name.

Renaming the underlying table doesn't seem like it helps at all.

...Robert