Proposal: knowing detail of config files via SQL

On 1/22/15 11:13 AM, Sawada Masahiko wrote:

Hi,

As per discussion
</messages/by-id/CAD21AoDkds8Oqbr199wwrCp7fiDvOw6bbb+CGdwQHUF+gX_bVg@mail.gmail.com&gt;,
I would like to proposal new view like pg_file_settings to know detail
of config file via SQL.

- Background
In 9.4 postgresql.auto.conf is added to support ALTER SYSTEM command
and that config file is loaded after whenever postgresql.conf is
loaded.
That is, postgresql.auto.conf is quite high priority so that the value
in postgresql.conf can not work at all if DBA set it manually.
ALTER SYSTEM RESET command can remove the unnecessary value in
postgresql.auto.conf but there are no way to know about where the
value has came from.
(They can only give the information about the setting in last file it
is present.)

- Solution
The patch not is implemented yet, just proposing now.
I'm imaging that we can have new pg_file_settings view has following
column to store current assigned value in config file.
- guc value name
- guc value
- config file path (e.g. /opt/data/postgresql.sql,
/opt/data/postgresql.auto.conf, /opt/hoge.conf)
This view could be convenient for DBA to decide if the
postgresql.auto.conf is useful or not and if it's not useful then DBA
could use ALTER SYSTEM .. RESET command to remove the same from
postgresql.auto.conf.

Would this view have a row for every option in a config file? IE: if you set something in both postgresql.conf and postgresql.auto.conf, would it show up twice? I think it should, and that there should be a way to see which setting is actually in effect.

It looks like you're attempting to handle #include, yes?

Also other idea is to add additional columns existing view
(pg_settings), according to prev discussion.

I think it would be useful to have a separate view that shows all occurrences of a setting. I recall some comment about source_file and source_line not always being correct in pg_settings; if that's true we should fix that.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

Sawada Masahiko <sawada.mshk@gmail.com> writes:

As per discussion
</messages/by-id/CAD21AoDkds8Oqbr199wwrCp7fiDvOw6bbb+CGdwQHUF+gX_bVg@mail.gmail.com&gt;,
I would like to proposal new view like pg_file_settings to know detail
of config file via SQL.

- Background
In 9.4 postgresql.auto.conf is added to support ALTER SYSTEM command
and that config file is loaded after whenever postgresql.conf is
loaded.
That is, postgresql.auto.conf is quite high priority so that the value
in postgresql.conf can not work at all if DBA set it manually.
ALTER SYSTEM RESET command can remove the unnecessary value in
postgresql.auto.conf but there are no way to know about where the
value has came from.
(They can only give the information about the setting in last file it
is present.)

- Solution
The patch not is implemented yet, just proposing now.
I'm imaging that we can have new pg_file_settings view has following
column to store current assigned value in config file.
- guc value name
- guc value
- config file path (e.g. /opt/data/postgresql.sql,
/opt/data/postgresql.auto.conf, /opt/hoge.conf)
This view could be convenient for DBA to decide if the
postgresql.auto.conf is useful or not and if it's not useful then DBA
could use ALTER SYSTEM .. RESET command to remove the same from
postgresql.auto.conf.

Also other idea is to add additional columns existing view
(pg_settings), according to prev discussion.

Please give me comments.

I still don't understand what problem you think needs to be solved.
It's already perfectly clear from the pg_settings view when a value
came from postgresql.auto.conf. For instance:

regression=# select name,setting,source,sourcefile,sourceline from pg_settings where name = 'TimeZone';
name | setting | source | sourcefile | sourceline
----------+------------+--------------------+-------------------------------------------------+------------
TimeZone | US/Eastern | configuration file | /home/postgres/testversion/data/postgresql.conf | 531
(1 row)

regression=# alter system set timezone = 'Asia/Shanghai';
ALTER SYSTEM
regression=# select pg_reload_conf();
pg_reload_conf
----------------
t
(1 row)

regression=# select name,setting,source,sourcefile,sourceline from pg_settings where name = 'TimeZone';
name | setting | source | sourcefile | sourceline
----------+---------------+--------------------+------------------------------------------------------+------------
TimeZone | Asia/Shanghai | configuration file | /home/postgres/testversion/data/postgresql.auto.conf | 3
(1 row)

regression=# alter system reset timezone;
ALTER SYSTEM
regression=# select pg_reload_conf();
pg_reload_conf
----------------
t
(1 row)

regression=# select name,setting,source,sourcefile,sourceline from pg_settings where name = 'TimeZone';
name | setting | source | sourcefile | sourceline
----------+------------+--------------------+-------------------------------------------------+------------
TimeZone | US/Eastern | configuration file | /home/postgres/testversion/data/postgresql.conf | 531
(1 row)

What else is needed?

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

Tom Lane-2 wrote

regression=# alter system reset timezone;
ALTER SYSTEM
regression=# select pg_reload_conf();

How does someone know that performing the above commands will result in the
TimeZone setting being changed from Asia/Shanghai to US/Eastern?

David J.

--
View this message in context: http://postgresql.nabble.com/Proposal-knowing-detail-of-config-files-via-SQL-tp5835075p5835142.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

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

David G Johnston <david.g.johnston@gmail.com> writes:

Tom Lane-2 wrote

regression=# alter system reset timezone;
ALTER SYSTEM
regression=# select pg_reload_conf();

How does someone know that performing the above commands will result in the
TimeZone setting being changed from Asia/Shanghai to US/Eastern?

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

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 Thu, Jan 22, 2015 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David G Johnston <david.g.johnston@gmail.com> writes:

Tom Lane-2 wrote

regression=# alter system reset timezone;
ALTER SYSTEM
regression=# select pg_reload_conf();

How does someone know that performing the above commands will result in

the

TimeZone setting being changed from Asia/Shanghai to US/Eastern?

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

​The proposal provides for SQL access to all possible sources of variable
value setting and, ideally, a means of ordering them in priority order, so
that a search for TimeZone would return two records, one for
postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and
2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that what
the value is in postgresql.conf that would become active. Furthermore, if
postgresql.conf has a setting AND there is a mapping in an #included file
that information would be accessible via SQL as well.

David J.
​

David Johnston <david.g.johnston@gmail.com> writes:

On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

​The proposal provides for SQL access to all possible sources of variable
value setting and, ideally, a means of ordering them in priority order, so
that a search for TimeZone would return two records, one for
postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and
2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that what
the value is in postgresql.conf that would become active. Furthermore, if
postgresql.conf has a setting AND there is a mapping in an #included file
that information would be accessible via SQL as well.

I know what the proposal is. What I am questioning is the use-case that
justifies having us build and support all this extra mechanism. How often
does anyone need to know what the "next down" variable value would be?
And if they do need to know whether a variable is set in postgresql.conf,
how often wouldn't they just resort to "grep" instead? (Among other
points, grep would succeed at noticing commented-out entries, which this
mechanism would not.)

GUC has existed in more or less its current state for about 15 years,
and I don't recall a lot of complaints that would be solved by this.
Furthermore, given that ALTER SYSTEM was sold to us as more or less
obsoleting manual editing of postgresql.conf, I rather doubt that it's
changed the basis of discussion all that much.

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 Thu, Jan 22, 2015 at 3:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

​The proposal provides for SQL access to all possible sources of variable
value setting and, ideally, a means of ordering them in priority order,

so

that a search for TimeZone would return two records, one for
postgresql.auto.conf and one for postgresql.conf - which are numbered 1

and

2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that

what

the value is in postgresql.conf that would become active. Furthermore,

if

postgresql.conf has a setting AND there is a mapping in an #included file
that information would be accessible via SQL as well.

I know what the proposal is. What I am questioning is the use-case that
justifies having us build and support all this extra mechanism. How often
does anyone need to know what the "next down" variable value would be?
And if they do need to know whether a variable is set in postgresql.conf,
how often wouldn't they just resort to "grep" instead? (Among other
points, grep would succeed at noticing commented-out entries, which this
mechanism would not.)

GUC has existed in more or less its current state for about 15 years,
and I don't recall a lot of complaints that would be solved by this.
Furthermore, given that ALTER SYSTEM was sold to us as more or less
obsoleting manual editing of postgresql.conf, I rather doubt that it's
changed the basis of discussion all that much.

​i doubt we'd actually see many complaints since, like you say, people are
likely to just use a different technique. The only thing PostgreSQL itself
needs to provide is a master inventory of seen/known settings; the user
interface can be left up to other layers (psql, pgadmin, extensions,
etc...). It falls into making the system more user friendly. But maybe
the answer for those users is that if you setup is so complex this would be
helpful you probably need to rethink your setup. Then again, if I only
interact with the via SQL at least can issue RESET ​and know the end result
- after a config reload - without having to log into the server and grep
the config file - or know what the system defaults are for settings that do
not appear explicitly in postgresql.conf; all without having to refer to
documentation as well.

I'm doubtful it is going to interest any of the core hackers to put this in
place but it at least warrants a couple of passes of brain-storming which
can then be memorialized on the Wiki-ToDo.

David J.

On 01/22/2015 02:09 PM, David Johnston wrote:

​The proposal provides for SQL access to all possible sources of
variable value setting and, ideally, a means of ordering them in
priority order, so that a search for TimeZone would return two records,
one for postgresql.auto.conf and one for postgresql.conf - which are
numbered 1 and 2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that
what the value is in postgresql.conf that would become active.
Furthermore, if postgresql.conf has a setting AND there is a mapping in
an #included file that information would be accessible via SQL as well.

Wow. Um, I can't imagine any use for that which would justify the
overhead. And I'm practically the "settings geek".

Note that a single file can have multiple copies of the same GUC, plus
there's GUCs set interactively, as well as in the user and database
properties. So you're looking at a lot of different "versions".

I think you're in a position of needing to interest someone else in this
issue enough to produce a patch to argue about. I'm not seeing a lot of
interest in it here.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
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, Jan 23, 2015 at 3:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I know what the proposal is. What I am questioning is the use-case that
justifies having us build and support all this extra mechanism. How often
does anyone need to know what the "next down" variable value would be?

I would say not that often as I think nobody changes the settings in
configuration files every now and then, but it could still be meaningful
in situations as described upthread by Sawada.

I think similar to this, pg_reload_conf() or many such things will be used
not that frequently, it seems to me that here important point to consider
is that whether such a view could serve any purpose for real users?

If we see multiple value for same config parameter was possible even
previous to Alter System and there doesn't seem to be much need/demand
for such a view and the reason could be that user has no way of changing
the setting at file level with command, but now as we provide a way it
could be useful in certain cases when user want to retain previous
values (value in postgresql.conf).

I understand that usage of such a view is not that high, but it could be
meaningful in some cases.

And if they do need to know whether a variable is set in postgresql.conf,
how often wouldn't they just resort to "grep" instead?

Do always user/dba (having superuser privilege) access to postgresql.conf
file? It seems to me even if they have access, getting the information via
SQL would be more appealing.

(Among other
points, grep would succeed at noticing commented-out entries, which this
mechanism would not.)

GUC has existed in more or less its current state for about 15 years,
and I don't recall a lot of complaints that would be solved by this.
Furthermore, given that ALTER SYSTEM was sold to us as more or less
obsoleting manual editing of postgresql.conf, I rather doubt that it's
changed the basis of discussion all that much.

By providing such a view I don't mean to recommend the user to change
the settings by editing postgresql.conf manually and then use Alter System
for other cases, rather it could be used for some cases like for default
values
or if in rare cases user has changed the parameters manually.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Jan 23, 2015 at 4:36 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

Would this view have a row for every option in a config file? IE: if you set
something in both postgresql.conf and postgresql.auto.conf, would it show up
twice? I think it should, and that there should be a way to see which
setting is actually in effect.

yes.

It looks like you're attempting to handle #include, yes?

Of course yes.

Also other idea is to add additional columns existing view
(pg_settings), according to prev discussion.

I think it would be useful to have a separate view that shows all
occurrences of a setting. I recall some comment about source_file and
source_line not always being correct in pg_settings; if that's true we
should fix that.

You mean that pg_settings view shows the variable in current session?
pg_settings view shows can be changed if user set the GUC parameter in
session or GUC parameter is set to role individually.
But I don't think pg_file_settings view has such a behavior.

Regards,

-------
Sawada Masahiko

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

On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

The proposal provides for SQL access to all possible sources of variable
value setting and, ideally, a means of ordering them in priority order, so
that a search for TimeZone would return two records, one for
postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and
2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that what
the value is in postgresql.conf that would become active. Furthermore, if
postgresql.conf has a setting AND there is a mapping in an #included file
that information would be accessible via SQL as well.

I know what the proposal is. What I am questioning is the use-case that
justifies having us build and support all this extra mechanism. How often
does anyone need to know what the "next down" variable value would be?

I believe I've run into situations where this would be useful. I
wouldn't be willing to put in the effort to do this myself, but I
wouldn't be prepared to vote against a high-quality patch that someone
else felt motivated to write.

--
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 Tue, Jan 27, 2015 at 3:34 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

The proposal provides for SQL access to all possible sources of variable
value setting and, ideally, a means of ordering them in priority order, so
that a search for TimeZone would return two records, one for
postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and
2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that what
the value is in postgresql.conf that would become active. Furthermore, if
postgresql.conf has a setting AND there is a mapping in an #included file
that information would be accessible via SQL as well.

I know what the proposal is. What I am questioning is the use-case that
justifies having us build and support all this extra mechanism. How often
does anyone need to know what the "next down" variable value would be?

I believe I've run into situations where this would be useful. I
wouldn't be willing to put in the effort to do this myself, but I
wouldn't be prepared to vote against a high-quality patch that someone
else felt motivated to write.

Attached patch adds new view pg_file_settings (WIP version).
This view behaves like followings,
[postgres][5432](1)=# select * from pg_file_settings ;
name | setting |
sourcefile
----------------------------+--------------------+--------------------------------------------------------
max_connections | 100 |
/home/masahiko/pgsql/master/3data/postgresql.conf
shared_buffers | 128MB |
/home/masahiko/pgsql/master/3data/postgresql.conf
dynamic_shared_memory_type | posix |
/home/masahiko/pgsql/master/3data/postgresql.conf
log_timezone | Japan |
/home/masahiko/pgsql/master/3data/postgresql.conf
datestyle | iso, mdy |
/home/masahiko/pgsql/master/3data/postgresql.conf
timezone | Japan |
/home/masahiko/pgsql/master/3data/postgresql.conf
lc_messages | C |
/home/masahiko/pgsql/master/3data/postgresql.conf
lc_monetary | C |
/home/masahiko/pgsql/master/3data/postgresql.conf
lc_numeric | C |
/home/masahiko/pgsql/master/3data/postgresql.conf
lc_time | C |
/home/masahiko/pgsql/master/3data/postgresql.conf
default_text_search_config | pg_catalog.english |
/home/masahiko/pgsql/master/3data/postgresql.conf
work_mem | 128MB |
/home/masahiko/pgsql/master/3data/hoge.conf
checkpoint_segments | 300 |
/home/masahiko/pgsql/master/3data/hoge.conf
enable_indexscan | off |
/home/masahiko/pgsql/master/3data/postgresql.auto.conf
work_mem | 64MB |
/home/masahiko/pgsql/master/3data/postgresql.auto.conf

[postgres][5432](1)=# select name, setting from pg_settings where name
= 'work_mem';
name | setting
----------+---------
work_mem | 65536
(1 row)

Above query result shows that both hoge.conf and postgresql.auto.conf
have same config parameter work_mem, and work_mem in auto.conf is
effecitve.
We can confirm these parameter to decide if the postgresql.auto.conf
is useful or not.

Regards,

-------
Sawada Masahiko

On Fri, Jan 30, 2015 at 09:38:10PM +0900, Sawada Masahiko wrote:

On Tue, Jan 27, 2015 at 3:34 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

The proposal provides for SQL access to all possible sources of variable
value setting and, ideally, a means of ordering them in priority order, so
that a search for TimeZone would return two records, one for
postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and
2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that what
the value is in postgresql.conf that would become active. Furthermore, if
postgresql.conf has a setting AND there is a mapping in an #included file
that information would be accessible via SQL as well.

I know what the proposal is. What I am questioning is the use-case that
justifies having us build and support all this extra mechanism. How often
does anyone need to know what the "next down" variable value would be?

I believe I've run into situations where this would be useful. I
wouldn't be willing to put in the effort to do this myself, but I
wouldn't be prepared to vote against a high-quality patch that someone
else felt motivated to write.

Attached patch adds new view pg_file_settings (WIP version).
This view behaves like followings,
[postgres][5432](1)=# select * from pg_file_settings ;
name | setting |
sourcefile
----------------------------+--------------------+--------------------------------------------------------
max_connections | 100 |
/home/masahiko/pgsql/master/3data/postgresql.conf
shared_buffers | 128MB |
/home/masahiko/pgsql/master/3data/postgresql.conf

This looks great!

Is there a reason not to have the sourcefile as a column in
pg_settings?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

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

On Sat, Jan 31, 2015 at 12:24 AM, David Fetter <david@fetter.org> wrote:

On Fri, Jan 30, 2015 at 09:38:10PM +0900, Sawada Masahiko wrote:

On Tue, Jan 27, 2015 at 3:34 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

The proposal provides for SQL access to all possible sources of variable
value setting and, ideally, a means of ordering them in priority order, so
that a search for TimeZone would return two records, one for
postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and
2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that what
the value is in postgresql.conf that would become active. Furthermore, if
postgresql.conf has a setting AND there is a mapping in an #included file
that information would be accessible via SQL as well.

I know what the proposal is. What I am questioning is the use-case that
justifies having us build and support all this extra mechanism. How often
does anyone need to know what the "next down" variable value would be?

I believe I've run into situations where this would be useful. I
wouldn't be willing to put in the effort to do this myself, but I
wouldn't be prepared to vote against a high-quality patch that someone
else felt motivated to write.

Attached patch adds new view pg_file_settings (WIP version).
This view behaves like followings,
[postgres][5432](1)=# select * from pg_file_settings ;
name | setting |
sourcefile
----------------------------+--------------------+--------------------------------------------------------
max_connections | 100 |
/home/masahiko/pgsql/master/3data/postgresql.conf
shared_buffers | 128MB |
/home/masahiko/pgsql/master/3data/postgresql.conf

This looks great!

Is there a reason not to have the sourcefile as a column in
pg_settings?

pg_file_settings view also has source file column same as pg_settings.
It might was hard to confirm that column in previous mail.
I put example of pg_file_settings again as follows.

[postgres][5432](1)=# select * from pg_file_settings where name = 'work_mem';
-[ RECORD 1 ]------------------------------------------------------
name | work_mem
setting | 128MB
sourcefile | /home/masahiko/pgsql/master/3data/hoge.conf
-[ RECORD 2 ]------------------------------------------------------
name | work_mem
setting | 64MB
sourcefile | /home/masahiko/pgsql/master/3data/postgresql.auto.conf

Regards,

-------
Sawada Masahiko

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

​This would default to being available to superusers only, right? Details
of the file system shouldn't be available to any random user.​

__________________________________________________________________________________
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
Mike.Blackwell@rrd.com
http://www.rrdonnelley.com

<http://www.rrdonnelley.com/&gt;
* <Mike.Blackwell@rrd.com>*

On Fri, Jan 30, 2015 at 9:50 AM, Sawada Masahiko <sawada.mshk@gmail.com>
wrote:

On Sat, Jan 31, 2015 at 12:58 AM, Mike Blackwell <mike.blackwell@rrd.com> wrote:

This would default to being available to superusers only, right? Details of
the file system shouldn't be available to any random user.

This WIP patch does not behave like that, but I agree.
This view would be effective combine with ALTER SYSTEM, and ALTER
SYSTEM command is executable to superuser only also.

Regards,

-------
Sawada Masahiko

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

On Sat, Jan 31, 2015 at 12:50:20AM +0900, Sawada Masahiko wrote:

On Sat, Jan 31, 2015 at 12:24 AM, David Fetter <david@fetter.org> wrote:

On Fri, Jan 30, 2015 at 09:38:10PM +0900, Sawada Masahiko wrote:

On Tue, Jan 27, 2015 at 3:34 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is that a requirement, and if so why? Because this proposal doesn't
guarantee any such knowledge AFAICS.

The proposal provides for SQL access to all possible sources of variable
value setting and, ideally, a means of ordering them in priority order, so
that a search for TimeZone would return two records, one for
postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and
2 respectively - so that in looking at that result if the
postgresql.auto.conf entry were to be removed the user would know that what
the value is in postgresql.conf that would become active. Furthermore, if
postgresql.conf has a setting AND there is a mapping in an #included file
that information would be accessible via SQL as well.

I know what the proposal is. What I am questioning is the use-case that
justifies having us build and support all this extra mechanism. How often
does anyone need to know what the "next down" variable value would be?

I believe I've run into situations where this would be useful. I
wouldn't be willing to put in the effort to do this myself, but I
wouldn't be prepared to vote against a high-quality patch that someone
else felt motivated to write.

Attached patch adds new view pg_file_settings (WIP version).
This view behaves like followings,
[postgres][5432](1)=# select * from pg_file_settings ;
name | setting |
sourcefile
----------------------------+--------------------+--------------------------------------------------------
max_connections | 100 |
/home/masahiko/pgsql/master/3data/postgresql.conf
shared_buffers | 128MB |
/home/masahiko/pgsql/master/3data/postgresql.conf

This looks great!

Is there a reason not to have the sourcefile as a column in
pg_settings?

pg_file_settings view also has source file column same as pg_settings.
It might was hard to confirm that column in previous mail.
I put example of pg_file_settings again as follows.

[postgres][5432](1)=# select * from pg_file_settings where name = 'work_mem';
-[ RECORD 1 ]------------------------------------------------------
name | work_mem
setting | 128MB
sourcefile | /home/masahiko/pgsql/master/3data/hoge.conf
-[ RECORD 2 ]------------------------------------------------------
name | work_mem
setting | 64MB
sourcefile | /home/masahiko/pgsql/master/3data/postgresql.auto.conf

Masuhiko-san,

I apologize for not communicating clearly. My alternative proposal is
to add a NULL-able sourcefile column to pg_settings. This is as an
alternative to creating a new pg_file_settings view.

Why might the contents of pg_settings be different from what would be
in pg_file_settings, apart from the existence of this column?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
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, Jan 30, 2015 at 12:05 PM, David Fetter <david@fetter.org> wrote:

On Sat, Jan 31, 2015 at 12:50:20AM +0900, Sawada Masahiko wrote:

[postgres][5432](1)=# select * from pg_file_settings where name =

'work_mem';

-[ RECORD 1 ]------------------------------------------------------
name | work_mem
setting | 128MB
sourcefile | /home/masahiko/pgsql/master/3data/hoge.conf
-[ RECORD 2 ]------------------------------------------------------
name | work_mem
setting | 64MB
sourcefile | /home/masahiko/pgsql/master/3data/postgresql.auto.conf

Masuhiko-san,

I apologize for not communicating clearly. My alternative proposal is
to add a NULL-able sourcefile column to pg_settings. This is as an
alternative to creating a new pg_file_settings view.

Why might the contents of pg_settings be different from what would be
in pg_file_settings, apart from the existence of this column?

​
​​The contents of pg_settings uses the setting name as a primary key.

The contents of pg_file_setting uses a compound key consisting of the
setting name and the source file.

See "work_mem" in the provided example.

More specifically pg_settings only care about the "currently active
setting" while this cares about "inactive" settings as well.

David J.
​

David Johnston <david.g.johnston@gmail.com> writes:

On Fri, Jan 30, 2015 at 12:05 PM, David Fetter <david@fetter.org> wrote:

Why might the contents of pg_settings be different from what would be
in pg_file_settings, apart from the existence of this column?

​​The contents of pg_settings uses the setting name as a primary key.
The contents of pg_file_setting uses a compound key consisting of the
setting name and the source file.

[ raised eyebrow... ] Seems insufficient in the case that multiple
settings of the same value occur in the same source file. Don't you
need a line number in the key as well?

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 Sat, Jan 31, 2015 at 4:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Fri, Jan 30, 2015 at 12:05 PM, David Fetter <david@fetter.org> wrote:

Why might the contents of pg_settings be different from what would be
in pg_file_settings, apart from the existence of this column?

The contents of pg_settings uses the setting name as a primary key.
The contents of pg_file_setting uses a compound key consisting of the
setting name and the source file.

[ raised eyebrow... ] Seems insufficient in the case that multiple
settings of the same value occur in the same source file. Don't you
need a line number in the key as well?

Yep, a line number is also needed.
The source file and line number would be primary key of pg_file_settings view.
I need to change like that.

Regards,

-------
Sawada Masahiko

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

On Sat, Jan 31, 2015 at 2:00 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:

On Sat, Jan 31, 2015 at 4:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Fri, Jan 30, 2015 at 12:05 PM, David Fetter <david@fetter.org> wrote:

Why might the contents of pg_settings be different from what would be
in pg_file_settings, apart from the existence of this column?

The contents of pg_settings uses the setting name as a primary key.
The contents of pg_file_setting uses a compound key consisting of the
setting name and the source file.

[ raised eyebrow... ] Seems insufficient in the case that multiple
settings of the same value occur in the same source file. Don't you
need a line number in the key as well?

Yep, a line number is also needed.
The source file and line number would be primary key of pg_file_settings view.
I need to change like that.

Attached patch is latest version including following changes.
- This view is available to super use only
- Add sourceline coulmn

Also I registered CF app.

Regards,

-------
Sawada Masahiko

On Sat, Jan 31, 2015 at 3:20 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:

On Sat, Jan 31, 2015 at 2:00 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:

On Sat, Jan 31, 2015 at 4:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Johnston <david.g.johnston@gmail.com> writes:

On Fri, Jan 30, 2015 at 12:05 PM, David Fetter <david@fetter.org> wrote:

Why might the contents of pg_settings be different from what would be
in pg_file_settings, apart from the existence of this column?

The contents of pg_settings uses the setting name as a primary key.
The contents of pg_file_setting uses a compound key consisting of the
setting name and the source file.

[ raised eyebrow... ] Seems insufficient in the case that multiple
settings of the same value occur in the same source file. Don't you
need a line number in the key as well?

Yep, a line number is also needed.
The source file and line number would be primary key of pg_file_settings view.
I need to change like that.

Attached patch is latest version including following changes.
- This view is available to super use only
- Add sourceline coulmn

Also I registered CF app.

Sorry, I registered unnecessary (extra) threads to CF app.
How can I delete them?

Regards,

-------
Sawada Masahiko

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

Sawada,

* Sawada Masahiko (sawada.mshk@gmail.com) wrote:

Attached patch is latest version including following changes.
- This view is available to super use only
- Add sourceline coulmn

Alright, first off, to Josh's point- I'm definitely interested in a
capability to show where the heck a given config value is coming from.
I'd be even happier if there was a way to *force* where config values
have to come from, but that ship sailed a year ago or so. Having this
be for the files, specifically, seems perfectly reasonable to me. I
could see having another function which will report where a currently
set GUC variable came from (eg: ALTER ROLE or ALTER DATABASE), but
having a function for "which config file is this GUC set in" seems
perfectly reasonable to me.

To that point, here's a review of this patch.

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 6df6bf2..f628cb0 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -412,6 +412,11 @@ CREATE RULE pg_settings_n AS

GRANT SELECT, UPDATE ON pg_settings TO PUBLIC;

+CREATE VIEW pg_file_settings AS
+   SELECT * FROM pg_show_all_file_settings() AS A;
+
+REVOKE ALL on pg_file_settings FROM public;

While this generally "works", the usual expectation is that functions
that should be superuser-only have a check in the function rather than
depending on the execute privilege. I'm certainly happy to debate the
merits of that approach, but for the purposes of this patch, I'd suggest
you stick an if (!superuser()) ereport("must be superuser") into the
function itself and not work about setting the correct permissions on
it.

+ ConfigFileVariable *guc;

As this ends up being an array, I'd call it "guc_array" or something
along those lines. GUC in PG-land has a pretty specific single-entity
meaning.

@@ -344,6 +346,21 @@ ProcessConfigFile(GucContext context)
PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT);
}

+	guc_file_variables = (ConfigFileVariable *)
+			guc_malloc(FATAL, num_guc_file_variables * sizeof(struct ConfigFileVariable));

Uh, perhaps I missed it, but what happens on a reload? Aren't we going
to realloc this every time? Seems like we should be doing a
guc_malloc() the first time through but doing guc_realloc() after, or
we'll leak memory on every reload.

+	/*
+	 * Apply guc config parameters to guc_file_variable
+	 */
+	guc = guc_file_variables;
+	for (item = head; item; item = item->next, guc++)
+	{
+	    guc->name = guc_strdup(FATAL, item->name);
+	    guc->value = guc_strdup(FATAL, item->value);
+	    guc->filename = guc_strdup(FATAL, item->filename);
+	    guc->sourceline = item->sourceline;
+	}

Uh, ditto and double-down here. I don't see a great solution other than
looping through the previous array and free'ing each of these, since we
can't depend on the memory context machinery being up and ready at this
point, as I recall.

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 931993c..c69ce66 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3561,9 +3561,17 @@ static const char *const map_old_guc_names[] = {
*/
static struct config_generic **guc_variables;

+/*
+ * lookup of variables for pg_file_settings view.
+ */
+static struct ConfigFileVariable *guc_file_variables;
+
/* Current number of variables contained in the vector */
static int	num_guc_variables;

+/* Number of file variables */
+static int	num_guc_file_variables;
+

I'd put these together, and add a comment explaining that
guc_file_variables is an array of length num_guc_file_variables..

/*
+ * Return the total number of GUC File variables
+ */
+int
+GetNumConfigFileOptions(void)
+{
+	return num_guc_file_variables;
+}

I don't see the point of this function..

+Datum
+show_all_file_settings(PG_FUNCTION_ARGS)
+{
+	FuncCallContext *funcctx;
+	TupleDesc	tupdesc;
+	int			call_cntr;
+	int			max_calls;
+	AttInMetadata *attinmeta;
+	MemoryContext oldcontext;
+
+	if (SRF_IS_FIRSTCALL())
+	{
+		funcctx = SRF_FIRSTCALL_INIT();
+
+		oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
+		/*
+		 * need a tuple descriptor representing NUM_PG_SETTINGS_ATTS columns
+		 * of the appropriate types
+		 */
+
+		tupdesc = CreateTemplateTupleDesc(NUM_PG_FILE_SETTINGS_ATTS, false);
+		TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
+						   TEXTOID, -1, 0);
+		TupleDescInitEntry(tupdesc, (AttrNumber) 2, "setting",
+						   TEXTOID, -1, 0);
+		TupleDescInitEntry(tupdesc, (AttrNumber) 3, "sourcefile",
+						   TEXTOID, -1, 0);
+		TupleDescInitEntry(tupdesc, (AttrNumber) 4, "sourceline",
+						   INT4OID, -1, 0);

As the sourcefile and sourceline were discussed as being the 'key' for
this, I expected them to be the first columns.. Any reason to not do
that? It's certainly look cleaner, at least to me, that way.

+	if (call_cntr < max_calls)
+	{
+		char	   *values[NUM_PG_FILE_SETTINGS_ATTS];
+		HeapTuple	tuple;
+		Datum		result;
+		ConfigFileVariable conf;
+		char		buffer[256];
+
+		conf = guc_file_variables[call_cntr];

I'm a bit nervous that a sighup in the middle could screw this up. Have
you considered that? At a minimum, I'd suggest a check along the lines
of:

if (call_cntr > num_guc_file_variables)
SRF_RETURNDONE();

To try and avoid going past the end of the array.

+		values[0] = conf.name;
+		values[1] = conf.value;
+		values[2] = conf.filename;
+		snprintf(buffer, sizeof(buffer), "%d", conf.sourceline);

Seems like we might be able to use pg_ltoa() there..

+ if (call_cntr >= max_calls)
+ SRF_RETURN_DONE(funcctx);

Isn't this redundant? We wouldn't be here if this was true.

Just a quick review.

Thanks!

Stephen

On Sat, Feb 28, 2015 at 12:27 AM, Stephen Frost <sfrost@snowman.net> wrote:

While this generally "works", the usual expectation is that functions
that should be superuser-only have a check in the function rather than
depending on the execute privilege. I'm certainly happy to debate the
merits of that approach, but for the purposes of this patch, I'd suggest
you stick an if (!superuser()) ereport("must be superuser") into the
function itself and not work about setting the correct permissions on
it.

-1. If that policy exists at all, it's a BAD policy, because it
prevents users from changing the permissions using DDL. I think the
superuser check should be inside the function, when, for example, it
masks some of the output data depending on the user's permissions.
But I see little virtue in handicapping an attempt by a superuser to
grant SELECT rights on pg_file_settings.

--
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 2/27/15 11:27 PM, Stephen Frost wrote:

@@ -344,6 +346,21 @@ ProcessConfigFile(GucContext context)
PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT);
}

+	guc_file_variables = (ConfigFileVariable *)
+			guc_malloc(FATAL, num_guc_file_variables * sizeof(struct ConfigFileVariable));

Uh, perhaps I missed it, but what happens on a reload? Aren't we going
to realloc this every time? Seems like we should be doing a
guc_malloc() the first time through but doing guc_realloc() after, or
we'll leak memory on every reload.

+	/*
+	 * Apply guc config parameters to guc_file_variable
+	 */
+	guc = guc_file_variables;
+	for (item = head; item; item = item->next, guc++)
+	{
+	    guc->name = guc_strdup(FATAL, item->name);
+	    guc->value = guc_strdup(FATAL, item->value);
+	    guc->filename = guc_strdup(FATAL, item->filename);
+	    guc->sourceline = item->sourceline;
+	}

Uh, ditto and double-down here. I don't see a great solution other than
looping through the previous array and free'ing each of these, since we
can't depend on the memory context machinery being up and ready at this
point, as I recall.

MemoryContextInit() happens near the top of main(), before we call
InitializeGUCOptions(). So it should be possible to use memory contexts
here. I don't know why guc doesn't use palloc; perhaps for historical
reasons at this point?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
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) wrote:

On Sat, Feb 28, 2015 at 12:27 AM, Stephen Frost <sfrost@snowman.net> wrote:

While this generally "works", the usual expectation is that functions
that should be superuser-only have a check in the function rather than
depending on the execute privilege. I'm certainly happy to debate the
merits of that approach, but for the purposes of this patch, I'd suggest
you stick an if (!superuser()) ereport("must be superuser") into the
function itself and not work about setting the correct permissions on
it.

-1. If that policy exists at all, it's a BAD policy, because it
prevents users from changing the permissions using DDL. I think the
superuser check should be inside the function, when, for example, it
masks some of the output data depending on the user's permissions.
But I see little virtue in handicapping an attempt by a superuser to
grant SELECT rights on pg_file_settings.

It's not a documented policy but it's certainly what a whole slew of
functions *do*. Including pg_start_backup, pg_stop_backup,
pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point,
pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume,
pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir.

Indeed, it's the larger portion of what the additional role attributes
are for. I'm not entirely thrilled to hear that nearly all of that work
might be moot, but if that's the case then so be it and let's just
remove all those superuser checks and instead revoke EXECUTE from public
and let the superuser grant out those rights.

As for pg_stat_get_wal_senders, pg_stat_get_activity, and proably some
others, column-level privileges and/or RLS policies might be able to
provide what we want there (or possibly not), but it's something to
consider if we want to go this route.

For pg_signal_backend, we could revoke direct EXECUTE permissions on it
and then keep just the check that says only superusers can signal
superuser initiated processes, and then a superuser could grant EXECUTE
rights on it directly for users they want to have that access. We could
possibly also create new helper functions for pg_terminate and
pg_cancel.

The discussion I'm having with Peter on another thread is a very similar
case that should be looping in, which is if we should continue to have
any superuser check on updating catalog tables. He is advocating that
we remove that also and let the superuser GRANT modification access on
catalog tables to users.

For my part, I understood that we specifically didn't want to allow that
for the same reason that we didn't want to simply depend on the GRANT
system for the above functions, but everyone else on these discussions
so far is advocating for using the GRANT system. My memory might be
wrong, but I could have sworn that I had brought up exactly that
suggestion in years past only to have it shot down.

Thanks,

Stephen

On Tue, Mar 3, 2015 at 10:29 PM, Stephen Frost <sfrost@snowman.net> wrote:

-1. If that policy exists at all, it's a BAD policy, because it
prevents users from changing the permissions using DDL. I think the
superuser check should be inside the function, when, for example, it
masks some of the output data depending on the user's permissions.
But I see little virtue in handicapping an attempt by a superuser to
grant SELECT rights on pg_file_settings.

It's not a documented policy but it's certainly what a whole slew of
functions *do*. Including pg_start_backup, pg_stop_backup,
pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point,
pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume,
pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir.

And the same issue comes up for the pg_hba_settings patch.

Fwiw it's not entirely true that users can't change the permissions on
these functions via DDL -- it's just a lot harder. They have to create
a security-definer wrapper function and then grant access to that
function to who they want.

I'm generally of the opinion we should use the GRANT system and not
hard-wire things but I also see the concern that it's really easy to
grant access to something without realizing all the consequences. It's
a lot harder to audit your system to be sure nothing inappropriate has
been granted which can be escalated to super-user access. It's not
clear how to determine what the set of things are that could do that.

--
greg

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

Stephen Frost <sfrost@snowman.net> writes:

It's not a documented policy but it's certainly what a whole slew of
functions *do*. Including pg_start_backup, pg_stop_backup,
pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point,
pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume,
pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir.

Indeed, it's the larger portion of what the additional role attributes
are for. I'm not entirely thrilled to hear that nearly all of that work
might be moot, but if that's the case then so be it and let's just
remove all those superuser checks and instead revoke EXECUTE from public
and let the superuser grant out those rights.

It does seem like that could be an easier and more flexible answer than
inventing a pile of role attributes.

The discussion I'm having with Peter on another thread is a very similar
case that should be looping in, which is if we should continue to have
any superuser check on updating catalog tables. He is advocating that
we remove that also and let the superuser GRANT modification access on
catalog tables to users.

For my part, I understood that we specifically didn't want to allow that
for the same reason that we didn't want to simply depend on the GRANT
system for the above functions, but everyone else on these discussions
so far is advocating for using the GRANT system. My memory might be
wrong, but I could have sworn that I had brought up exactly that
suggestion in years past only to have it shot down.

I'm a bit less comfortable with this one, mainly because the ability to
modify the system catalogs directly implies the ability to crash, corrupt,
or hijack the database in any number of non-obvious ways. I would go so
far as to argue that if you grant me modify permissions on many of the
catalogs, I would have the ability to become superuser outright, and
therefore any notion that such a grant is any weaker than granting full
superuserness is a dangerous lie.

It may be that the same type of permissions escalation is possible with
some of the functions you mention above; but anywhere that it isn't,
I should think GRANT on the function is a reasonable solution.

One aspect of this that merits some thought is that in some cases
access to some set of functions is best granted as a unit. That's
easy with role properties but much less so with plain GRANT.
Do we have enough such cases to make it an issue?

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

The discussion I'm having with Peter on another thread is a very similar
case that should be looping in, which is if we should continue to have
any superuser check on updating catalog tables. He is advocating that
we remove that also and let the superuser GRANT modification access on
catalog tables to users.

For my part, I understood that we specifically didn't want to allow that
for the same reason that we didn't want to simply depend on the GRANT
system for the above functions, but everyone else on these discussions
so far is advocating for using the GRANT system. My memory might be
wrong, but I could have sworn that I had brought up exactly that
suggestion in years past only to have it shot down.

I'm a bit less comfortable with this one, mainly because the ability to
modify the system catalogs directly implies the ability to crash, corrupt,
or hijack the database in any number of non-obvious ways. I would go so
far as to argue that if you grant me modify permissions on many of the
catalogs, I would have the ability to become superuser outright, and
therefore any notion that such a grant is any weaker than granting full
superuserness is a dangerous lie.

I tend to agree with this approach and it's what I was advocating for in
my overall analysis of superuser checks that gave rise to the additional
role attributes idea. If it can be used to become superuser then it
doesn't make sense to have it be independent from superuser.

It may be that the same type of permissions escalation is possible with
some of the functions you mention above; but anywhere that it isn't,
I should think GRANT on the function is a reasonable solution.

The functions identified for the new role attributes were specifically
those that, I believe, could be used without also giving the user
superuser rights. Those are:

pg_start_backup, pg_stop_backup, pg_switch_xlog, pg_rotate_logfile,
pg_create_restore_point, pg_xlog_replay_pause, lo_import, lo_export,
and pg_xlog_replay_resume.

One aspect of this that merits some thought is that in some cases
access to some set of functions is best granted as a unit. That's
easy with role properties but much less so with plain GRANT.
Do we have enough such cases to make it an issue?

That's what roles are for, in my mind. If we're fine with using the
grant system to control executing these functions then I would suggest
we address this by providing some pre-configured roles or even just
documentation which list what a given role would need. That's
certainly what a lot of people coming from other databases expect. The
problem with the role attribute approach is that they aren't inheirted
the way GRANTs are, which means you can't have a "backup" role that is
then granted out to users, you'd have to set a "BACKUP" role attribute
for every role added.

Thanks,

Stephen

On 3/3/15 5:22 PM, Stephen Frost wrote:

The
problem with the role attribute approach is that they aren't inheirted
the way GRANTs are, which means you can't have a "backup" role that is
then granted out to users, you'd have to set a "BACKUP" role attribute
for every role added.

Yeah, but you'd still have to grant "backup" to every role created
anyway, right?

Or you could create a role that has the backup attribute and then grant
that to users. Then they'd have to intentionally SET ROLE my_backup_role
to elevate their privilege. That seems like a safer way to do things...
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

Jim,

* Jim Nasby (Jim.Nasby@BlueTreble.com) wrote:

On 3/3/15 5:22 PM, Stephen Frost wrote:

The
problem with the role attribute approach is that they aren't inheirted
the way GRANTs are, which means you can't have a "backup" role that is
then granted out to users, you'd have to set a "BACKUP" role attribute
for every role added.

Yeah, but you'd still have to grant "backup" to every role created
anyway, right?

Yes, you would.

Or you could create a role that has the backup attribute and then
grant that to users. Then they'd have to intentionally SET ROLE
my_backup_role to elevate their privilege. That seems like a safer
way to do things...

This is already possible with the GRANT system- create a 'noinherit'
role instead of an 'inherit' role. I agree it's safer to require a
'SET ROLE' and configure all of my systems with a noinherit 'admin'
role.

Thanks!

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:

pg_start_backup, pg_stop_backup, pg_switch_xlog, pg_rotate_logfile,
pg_create_restore_point, pg_xlog_replay_pause, lo_import, lo_export,
and pg_xlog_replay_resume.

Meh, that list was too hastily copied and pasted from my earlier email.
lo_import and lo_export were *not* included in the role attributes list,
nor would I advocate making them GRANT'able.

Thanks!

Stephen

On 3/2/15 4:47 PM, Robert Haas wrote:

On Sat, Feb 28, 2015 at 12:27 AM, Stephen Frost <sfrost@snowman.net> wrote:

While this generally "works", the usual expectation is that functions
that should be superuser-only have a check in the function rather than
depending on the execute privilege. I'm certainly happy to debate the
merits of that approach, but for the purposes of this patch, I'd suggest
you stick an if (!superuser()) ereport("must be superuser") into the
function itself and not work about setting the correct permissions on
it.

-1. If that policy exists at all, it's a BAD policy, because it
prevents users from changing the permissions using DDL. I think the
superuser check should be inside the function, when, for example, it
masks some of the output data depending on the user's permissions.
But I see little virtue in handicapping an attempt by a superuser to
grant SELECT rights on pg_file_settings.

This is in fact how most if not all code ensures supervisor-only access
to functions, so for the purpose of this patch, I think it is the
correct approach. Someone may well change that soon after, if the other
ongoing efforts conclude.

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

On 3/3/15 5:58 PM, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

It's not a documented policy but it's certainly what a whole slew of
functions *do*. Including pg_start_backup, pg_stop_backup,
pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point,
pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume,
pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir.

Indeed, it's the larger portion of what the additional role attributes
are for. I'm not entirely thrilled to hear that nearly all of that work
might be moot, but if that's the case then so be it and let's just
remove all those superuser checks and instead revoke EXECUTE from public
and let the superuser grant out those rights.

It does seem like that could be an easier and more flexible answer than
inventing a pile of role attributes.

One issue is that privilege changes on built-in stuff (and extensions)
are not dumped. But that is fixable.

One aspect of this that merits some thought is that in some cases
access to some set of functions is best granted as a unit. That's
easy with role properties but much less so with plain GRANT.
Do we have enough such cases to make it an issue?

You could have built-in roles, such as "backup" and ship the system with
the "backup" role having permissions on some functions. And then users
are granted those roles. Similar to how some Linux systems ship with
groups such as "adm".

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

On 3/3/15 5:29 PM, Stephen Frost wrote:

For my part, I understood that we specifically didn't want to allow that
for the same reason that we didn't want to simply depend on the GRANT
system for the above functions, but everyone else on these discussions
so far is advocating for using the GRANT system. My memory might be
wrong, but I could have sworn that I had brought up exactly that
suggestion in years past only to have it shot down.

I think a lot of this access control work is done based on some
undocumented understandings, when in fact there is no consensus on
anything. I think we need more clarity.

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

On Sat, Feb 28, 2015 at 2:27 PM, Stephen Frost <sfrost@snowman.net> wrote:

Sawada,

* Sawada Masahiko (sawada.mshk@gmail.com) wrote:

Attached patch is latest version including following changes.
- This view is available to super use only
- Add sourceline coulmn

Alright, first off, to Josh's point- I'm definitely interested in a
capability to show where the heck a given config value is coming from.
I'd be even happier if there was a way to *force* where config values
have to come from, but that ship sailed a year ago or so. Having this
be for the files, specifically, seems perfectly reasonable to me. I
could see having another function which will report where a currently
set GUC variable came from (eg: ALTER ROLE or ALTER DATABASE), but
having a function for "which config file is this GUC set in" seems
perfectly reasonable to me.

To that point, here's a review of this patch.

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 6df6bf2..f628cb0 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -412,6 +412,11 @@ CREATE RULE pg_settings_n AS

GRANT SELECT, UPDATE ON pg_settings TO PUBLIC;

+CREATE VIEW pg_file_settings AS
+   SELECT * FROM pg_show_all_file_settings() AS A;
+
+REVOKE ALL on pg_file_settings FROM public;

While this generally "works", the usual expectation is that functions
that should be superuser-only have a check in the function rather than
depending on the execute privilege. I'm certainly happy to debate the
merits of that approach, but for the purposes of this patch, I'd suggest
you stick an if (!superuser()) ereport("must be superuser") into the
function itself and not work about setting the correct permissions on
it.

+ ConfigFileVariable *guc;

As this ends up being an array, I'd call it "guc_array" or something
along those lines. GUC in PG-land has a pretty specific single-entity
meaning.

@@ -344,6 +346,21 @@ ProcessConfigFile(GucContext context)
PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT);
}

+     guc_file_variables = (ConfigFileVariable *)
+                     guc_malloc(FATAL, num_guc_file_variables * sizeof(struct ConfigFileVariable));

Uh, perhaps I missed it, but what happens on a reload? Aren't we going
to realloc this every time? Seems like we should be doing a
guc_malloc() the first time through but doing guc_realloc() after, or
we'll leak memory on every reload.

+     /*
+      * Apply guc config parameters to guc_file_variable
+      */
+     guc = guc_file_variables;
+     for (item = head; item; item = item->next, guc++)
+     {
+         guc->name = guc_strdup(FATAL, item->name);
+         guc->value = guc_strdup(FATAL, item->value);
+         guc->filename = guc_strdup(FATAL, item->filename);
+         guc->sourceline = item->sourceline;
+     }

Uh, ditto and double-down here. I don't see a great solution other than
looping through the previous array and free'ing each of these, since we
can't depend on the memory context machinery being up and ready at this
point, as I recall.

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 931993c..c69ce66 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3561,9 +3561,17 @@ static const char *const map_old_guc_names[] = {
*/
static struct config_generic **guc_variables;

+/*
+ * lookup of variables for pg_file_settings view.
+ */
+static struct ConfigFileVariable *guc_file_variables;
+
/* Current number of variables contained in the vector */
static int   num_guc_variables;

+/* Number of file variables */
+static int   num_guc_file_variables;
+

I'd put these together, and add a comment explaining that
guc_file_variables is an array of length num_guc_file_variables..

/*
+ * Return the total number of GUC File variables
+ */
+int
+GetNumConfigFileOptions(void)
+{
+     return num_guc_file_variables;
+}

I don't see the point of this function..

+Datum
+show_all_file_settings(PG_FUNCTION_ARGS)
+{
+     FuncCallContext *funcctx;
+     TupleDesc       tupdesc;
+     int                     call_cntr;
+     int                     max_calls;
+     AttInMetadata *attinmeta;
+     MemoryContext oldcontext;
+
+     if (SRF_IS_FIRSTCALL())
+     {
+             funcctx = SRF_FIRSTCALL_INIT();
+
+             oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
+             /*
+              * need a tuple descriptor representing NUM_PG_SETTINGS_ATTS columns
+              * of the appropriate types
+              */
+
+             tupdesc = CreateTemplateTupleDesc(NUM_PG_FILE_SETTINGS_ATTS, false);
+             TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
+                                                TEXTOID, -1, 0);
+             TupleDescInitEntry(tupdesc, (AttrNumber) 2, "setting",
+                                                TEXTOID, -1, 0);
+             TupleDescInitEntry(tupdesc, (AttrNumber) 3, "sourcefile",
+                                                TEXTOID, -1, 0);
+             TupleDescInitEntry(tupdesc, (AttrNumber) 4, "sourceline",
+                                                INT4OID, -1, 0);

As the sourcefile and sourceline were discussed as being the 'key' for
this, I expected them to be the first columns.. Any reason to not do
that? It's certainly look cleaner, at least to me, that way.

+     if (call_cntr < max_calls)
+     {
+             char       *values[NUM_PG_FILE_SETTINGS_ATTS];
+             HeapTuple       tuple;
+             Datum           result;
+             ConfigFileVariable conf;
+             char            buffer[256];
+
+             conf = guc_file_variables[call_cntr];

I'm a bit nervous that a sighup in the middle could screw this up. Have
you considered that? At a minimum, I'd suggest a check along the lines
of:

if (call_cntr > num_guc_file_variables)
SRF_RETURNDONE();

To try and avoid going past the end of the array.

+             values[0] = conf.name;
+             values[1] = conf.value;
+             values[2] = conf.filename;
+             snprintf(buffer, sizeof(buffer), "%d", conf.sourceline);

Seems like we might be able to use pg_ltoa() there..

+             if (call_cntr >= max_calls)
+                     SRF_RETURN_DONE(funcctx);

Isn't this redundant? We wouldn't be here if this was true.

Just a quick review.

Thank you for your review!
Attached file is the latest version (without document patch. I making it now.)
As per discussion, there is no change regarding of super user permission.

--
Regards,

-------
Sawada Masahiko

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 3/3/15 5:29 PM, Stephen Frost wrote:

For my part, I understood that we specifically didn't want to allow that
for the same reason that we didn't want to simply depend on the GRANT
system for the above functions, but everyone else on these discussions
so far is advocating for using the GRANT system. My memory might be
wrong, but I could have sworn that I had brought up exactly that
suggestion in years past only to have it shot down.

I think a lot of this access control work is done based on some
undocumented understandings, when in fact there is no consensus on
anything. I think we need more clarity.

When there hasn't been discussion on a particular topic and years of
ongoing development, all of which uses one approach, I tend to figure
that makes it an unspoking consensus. I'm not saying we shouldn't
question that when it makes sense to, we certainly should, but I'm not
sure it makes sense to ask "why didn't you attempt to get consensus on
this thing we've all been doing for years?"

Thanks,

Stephen

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 3/3/15 5:58 PM, Tom Lane wrote:

One aspect of this that merits some thought is that in some cases
access to some set of functions is best granted as a unit. That's
easy with role properties but much less so with plain GRANT.
Do we have enough such cases to make it an issue?

You could have built-in roles, such as "backup" and ship the system with
the "backup" role having permissions on some functions. And then users
are granted those roles. Similar to how some Linux systems ship with
groups such as "adm".

One thought I had for this was a contrib module which added an extension
to create and grant those roles. That approach would mean that we don't
need to worry about upgrade-path problems which we could get into if we
declared new roles like 'backup' which users might already have.

An alternative approach which might be better, now that I think about
it, would be to declare that the 'pg_' prefix applies to roles too and
then have a 'pg_backup' role which is granted the correct permissions.
Personally, I like that idea a lot..

We could then have pg_upgrade throw an error and pg_dump a warning (or
something along those lines) if they find any existing roles with that
prefix.

Thanks!

Stephen

Sawada,

* Sawada Masahiko (sawada.mshk@gmail.com) wrote:

Thank you for your review!
Attached file is the latest version (without document patch. I making it now.)
As per discussion, there is no change regarding of super user permission.

Ok. Here's another review.

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5e69e2b..94ee229 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -414,6 +414,11 @@ CREATE RULE pg_settings_n AS

GRANT SELECT, UPDATE ON pg_settings TO PUBLIC;

+CREATE VIEW pg_file_settings AS
+   SELECT * FROM pg_show_all_file_settings() AS A;
+
+REVOKE ALL on pg_file_settings FROM public;
+

This suffers from a lack of pg_dump support, presuming that the
superuser is entitled to change the permissions on this function. As it
turns out though, you're in luck in that regard since I'm working on
fixing that for a mostly unrelated patch. Any feedback you have on what
I'm doing to pg_dump here:

/messages/by-id/20150307213935.GO29780@tamriel.snowman.net

Would certainly be appreciated.

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l

[...]

+	 * Calculate size of guc_array and allocate it. From the secound time to allcate memory,
+	 * we should free old allocated memory.
+	 */
+	guc_array_size = num_guc_file_variables * sizeof(struct ConfigFileVariable);
+	if (!guc_file_variables)
+	{
+		/* For the first call */
+		guc_file_variables = (ConfigFileVariable *) guc_malloc(FATAL, guc_array_size);
+	}
+	else
+	{
+		guc_array = guc_file_variables;
+		for (item = head; item; item = item->next, guc_array++)
+		{
+			free(guc_array->name);
+			free(guc_array->value);
+			free(guc_array->filename);

It's a minor nit-pick, as the below loop should handle anything we care
about, but I'd go ahead and reset guc_array->sourceline to be -1 or
something too, just to show that everything's being handled here with
regard to cleanup. Or perhaps just add a comment saying that the
sourceline for all currently valid entries will be updated.

+		guc_file_variables = (ConfigFileVariable *) guc_realloc(FATAL, guc_file_variables, guc_array_size);
+	}

Nasby made a comment, I believe, that we might actually be able to use
memory contexts here, which would certainly be much nicer as then you'd
be able to just throw away the whole context and create a new one. Have
you looked at that approach at all? Offhand, I'm not sure if it'd work
or not (I have my doubts..) but it seems worthwhile to consider.

Otherwise, it seems like this would address my previous concerns that
this would end up leaking memory, and even if it's a bit slower than one
might hope, it's not an operation we should be doing very frequently.

--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c

[...]

/*
+ * Return the total number of GUC File variables
+ */
+int
+GetNumConfigFileOptions(void)
+{
+	return num_guc_file_variables;
+}

What's the point of this function..? I'm not convinced it's the best
idea, but it certainly looks like the function and the variable are only
used from this file anyway?

+	if (call_cntr < max_calls)
+	{
+		char	   *values[NUM_PG_FILE_SETTINGS_ATTS];
+		HeapTuple	tuple;
+		Datum		result;
+		ConfigFileVariable conf;
+		char		buffer[256];

Isn't that buffer a bit.. excessive in size?

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d3100d1..ebb96cc 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -133,6 +133,14 @@ typedef struct ConfigVariable
struct ConfigVariable *next;
} ConfigVariable;

+typedef struct ConfigFileVariable
+{
+	char	*name;
+	char	*value;
+	char	*filename;
+	int		sourceline;
+} ConfigFileVariable;
+

[...]

+extern int GetNumConfigFileOptions(void);

These aren't actually used anywhere else, are they? Not sure that
there's any need to expose them beyond guc.c when that's the only place
they're used.

Thanks!

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:

--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -414,6 +414,11 @@ CREATE RULE pg_settings_n AS

GRANT SELECT, UPDATE ON pg_settings TO PUBLIC;

+CREATE VIEW pg_file_settings AS
+   SELECT * FROM pg_show_all_file_settings() AS A;
+
+REVOKE ALL on pg_file_settings FROM public;
+

Err, and further, I realize that you're not actually changing the
permissions on the actual function at all, which means that they're the
default which is "executable by anyone."

This will also need a

REVOKE EXECUTE on pg_show_all_file_settings() FROM public;

Or someone could simply run the function instead of using the view to
see the data returned.

Thanks,

Stephen

On Tue, Mar 10, 2015 at 12:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

Sawada,

* Sawada Masahiko (sawada.mshk@gmail.com) wrote:

Thank you for your review!
Attached file is the latest version (without document patch. I making it now.)
As per discussion, there is no change regarding of super user permission.

Ok. Here's another review.

Thank you for your review!

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5e69e2b..94ee229 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -414,6 +414,11 @@ CREATE RULE pg_settings_n AS

GRANT SELECT, UPDATE ON pg_settings TO PUBLIC;

+CREATE VIEW pg_file_settings AS
+   SELECT * FROM pg_show_all_file_settings() AS A;
+
+REVOKE ALL on pg_file_settings FROM public;
+

This suffers from a lack of pg_dump support, presuming that the
superuser is entitled to change the permissions on this function. As it
turns out though, you're in luck in that regard since I'm working on
fixing that for a mostly unrelated patch. Any feedback you have on what
I'm doing to pg_dump here:

/messages/by-id/20150307213935.GO29780@tamriel.snowman.net

Would certainly be appreciated.

Thank you for the info.
I will read the discussion and send the feedbacks.

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l

[...]

+      * Calculate size of guc_array and allocate it. From the secound time to allcate memory,
+      * we should free old allocated memory.
+      */
+     guc_array_size = num_guc_file_variables * sizeof(struct ConfigFileVariable);
+     if (!guc_file_variables)
+     {
+             /* For the first call */
+             guc_file_variables = (ConfigFileVariable *) guc_malloc(FATAL, guc_array_size);
+     }
+     else
+     {
+             guc_array = guc_file_variables;
+             for (item = head; item; item = item->next, guc_array++)
+             {
+                     free(guc_array->name);
+                     free(guc_array->value);
+                     free(guc_array->filename);

It's a minor nit-pick, as the below loop should handle anything we care
about, but I'd go ahead and reset guc_array->sourceline to be -1 or
something too, just to show that everything's being handled here with
regard to cleanup. Or perhaps just add a comment saying that the
sourceline for all currently valid entries will be updated.

Fixed.

+             guc_file_variables = (ConfigFileVariable *) guc_realloc(FATAL, guc_file_variables, guc_array_size);
+     }

Nasby made a comment, I believe, that we might actually be able to use
memory contexts here, which would certainly be much nicer as then you'd
be able to just throw away the whole context and create a new one. Have
you looked at that approach at all? Offhand, I'm not sure if it'd work
or not (I have my doubts..) but it seems worthwhile to consider.

I successfully used palloc() and pfree() when allocate and free
guc_file_variable,
but these variable seems to be freed already when I get value of
pg_file_settings view via SQL.

Otherwise, it seems like this would address my previous concerns that
this would end up leaking memory, and even if it's a bit slower than one
might hope, it's not an operation we should be doing very frequently.

--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c

[...]

/*
+ * Return the total number of GUC File variables
+ */
+int
+GetNumConfigFileOptions(void)
+{
+     return num_guc_file_variables;
+}

What's the point of this function..? I'm not convinced it's the best
idea, but it certainly looks like the function and the variable are only
used from this file anyway?

It's unnecessary.
Fixed.

+     if (call_cntr < max_calls)
+     {
+             char       *values[NUM_PG_FILE_SETTINGS_ATTS];
+             HeapTuple       tuple;
+             Datum           result;
+             ConfigFileVariable conf;
+             char            buffer[256];

Isn't that buffer a bit.. excessive in size?

Fixed.

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d3100d1..ebb96cc 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -133,6 +133,14 @@ typedef struct ConfigVariable
struct ConfigVariable *next;
} ConfigVariable;

+typedef struct ConfigFileVariable
+{
+     char    *name;
+     char    *value;
+     char    *filename;
+     int             sourceline;
+} ConfigFileVariable;
+

[...]

+extern int GetNumConfigFileOptions(void);

These aren't actually used anywhere else, are they? Not sure that
there's any need to expose them beyond guc.c when that's the only place
they're used.

Fixed.

This will also need a
REVOKE EXECUTE on pg_show_all_file_settings() FROM public;

Added.

Regards,

-------
Sawada Masahiko

On Wed, Mar 11, 2015 at 11:46 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:

On Tue, Mar 10, 2015 at 12:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

Sawada,

* Sawada Masahiko (sawada.mshk@gmail.com) wrote:

Thank you for your review!
Attached file is the latest version (without document patch. I making it now.)
As per discussion, there is no change regarding of super user permission.

Ok. Here's another review.

Thank you for your review!

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5e69e2b..94ee229 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -414,6 +414,11 @@ CREATE RULE pg_settings_n AS

GRANT SELECT, UPDATE ON pg_settings TO PUBLIC;

+CREATE VIEW pg_file_settings AS
+   SELECT * FROM pg_show_all_file_settings() AS A;
+
+REVOKE ALL on pg_file_settings FROM public;
+

This suffers from a lack of pg_dump support, presuming that the
superuser is entitled to change the permissions on this function. As it
turns out though, you're in luck in that regard since I'm working on
fixing that for a mostly unrelated patch. Any feedback you have on what
I'm doing to pg_dump here:

/messages/by-id/20150307213935.GO29780@tamriel.snowman.net

Would certainly be appreciated.

Thank you for the info.
I will read the discussion and send the feedbacks.

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l

[...]

+      * Calculate size of guc_array and allocate it. From the secound time to allcate memory,
+      * we should free old allocated memory.
+      */
+     guc_array_size = num_guc_file_variables * sizeof(struct ConfigFileVariable);
+     if (!guc_file_variables)
+     {
+             /* For the first call */
+             guc_file_variables = (ConfigFileVariable *) guc_malloc(FATAL, guc_array_size);
+     }
+     else
+     {
+             guc_array = guc_file_variables;
+             for (item = head; item; item = item->next, guc_array++)
+             {
+                     free(guc_array->name);
+                     free(guc_array->value);
+                     free(guc_array->filename);

It's a minor nit-pick, as the below loop should handle anything we care
about, but I'd go ahead and reset guc_array->sourceline to be -1 or
something too, just to show that everything's being handled here with
regard to cleanup. Or perhaps just add a comment saying that the
sourceline for all currently valid entries will be updated.

Fixed.

+             guc_file_variables = (ConfigFileVariable *) guc_realloc(FATAL, guc_file_variables, guc_array_size);
+     }

Nasby made a comment, I believe, that we might actually be able to use
memory contexts here, which would certainly be much nicer as then you'd
be able to just throw away the whole context and create a new one. Have
you looked at that approach at all? Offhand, I'm not sure if it'd work
or not (I have my doubts..) but it seems worthwhile to consider.

I successfully used palloc() and pfree() when allocate and free
guc_file_variable,
but these variable seems to be freed already when I get value of
pg_file_settings view via SQL.

Otherwise, it seems like this would address my previous concerns that
this would end up leaking memory, and even if it's a bit slower than one
might hope, it's not an operation we should be doing very frequently.

--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c

[...]

/*
+ * Return the total number of GUC File variables
+ */
+int
+GetNumConfigFileOptions(void)
+{
+     return num_guc_file_variables;
+}

What's the point of this function..? I'm not convinced it's the best
idea, but it certainly looks like the function and the variable are only
used from this file anyway?

It's unnecessary.
Fixed.

+     if (call_cntr < max_calls)
+     {
+             char       *values[NUM_PG_FILE_SETTINGS_ATTS];
+             HeapTuple       tuple;
+             Datum           result;
+             ConfigFileVariable conf;
+             char            buffer[256];

Isn't that buffer a bit.. excessive in size?

Fixed.

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d3100d1..ebb96cc 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -133,6 +133,14 @@ typedef struct ConfigVariable
struct ConfigVariable *next;
} ConfigVariable;

+typedef struct ConfigFileVariable
+{
+     char    *name;
+     char    *value;
+     char    *filename;
+     int             sourceline;
+} ConfigFileVariable;
+

[...]

+extern int GetNumConfigFileOptions(void);

These aren't actually used anywhere else, are they? Not sure that
there's any need to expose them beyond guc.c when that's the only place
they're used.

Fixed.

This will also need a
REVOKE EXECUTE on pg_show_all_file_settings() FROM public;

Added.

I added documentation changes to patch is attached.
Also I tried to use memory context for allocation of guc_file_variable
in TopMemoryContext,
but it was failed access after received SIGHUP.

Please review.

Regards,

-------
Sawada Masahiko

On 4/4/15 9:21 AM, Sawada Masahiko wrote:

I added documentation changes to patch is attached.
Also I tried to use memory context for allocation of guc_file_variable
in TopMemoryContext,
but it was failed access after received SIGHUP.

Below is my review of the v5 patch:

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
+ <sect1 id="view-pg-file-settings">
+  <title><structname>pg_file_settings</structname></title>
+
+  <indexterm zone="view-pg-file-settings">
+   <primary>pg_file_settings</primary>
+  </indexterm>
+
+  <para>
+   The view <structname>pg_file_settings</structname> provides confirm
parameters via SQL,
+   which is written in configuration file. This view can be update by
reloading configration file.
+  </para>

Perhaps something like:

<para>
The view <structname>pg_file_settings</structname> provides access to
run-time parameters
that are defined in configuration files via SQL. In contrast to
<structname>pg_settings</structname> a row is provided for each
occurrence
of the parameter in a configuration file. This is helpful for
discovering why one value
may have been used in preference to another when the parameters were
loaded.
</para>

+  <table>
+   <title><structname>pg_file_settings</> Columns</title>
+
+  <tgroup cols="3">
+   <thead>
+    <row>
+     <entry>Name</entry>
+     <entry>Type</entry>
+     <entry>Description</entry>
+    </row>
+   </thead>
+   <tbody>
+    <row>
+     <entry><structfield>sourcefile</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>A path of configration file</entry>

From pg_settings:

<entry>Configuration file the current value was set in (null for
values set from sources other than configuration files, or when
examined by a non-superuser);
helpful when using <literal>include</> directives in configuration
files</entry>

+    </row>
+    <row>
+     <entry><structfield>sourceline</structfield></entry>
+     <entry><structfield>integer</structfield></entry>
+     <entry>The line number in configuration file</entry>

From pg_settings:

<entry>Line number within the configuration file the current value was
set at (null for values set from sources other than configuration
files,
or when examined by a non-superuser)
</entry>

+    </row>
+    <row>
+     <entry><structfield>name</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>Parameter name in configuration file</entry>

From pg_settings:

<entry>Run-time configuration parameter name</entry>

+    </row>
+    <row>
+     <entry><structfield>setting</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>Value of the parameter in configuration file</entry>
+    </row>
+   </tbody>
+  </tgroup>
+ </table>
+
+</sect1>

diff --git a/src/backend/utils/misc/guc-file.l
b/src/backend/utils/misc/guc-file.l
@@ -342,6 +345,42 @@ ProcessConfigFile(GucContext context)
+        guc_array = guc_file_variables;
+        for (item = head; item; item = item->next, guc_array++)
+        {
+            free(guc_array->name);
+            free(guc_array->value);
+            free(guc_array->filename);

There's an issue freeing these when calling pg_reload_conf():

postgres=# alter system set max_connections = 100;
ALTER SYSTEM
postgres=# select * from pg_reload_conf();
LOG: received SIGHUP, reloading configuration files
pg_reload_conf
----------------
t
(1 row)

postgres=# postgres(25078,0x7fff747b2300) malloc: *** error for object
0x424d380044: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

Of course a config reload can't change max_connections, but I wouldn't
expect it to crash, either.

+ guc_array->sourceline = -1;

I can't see the need for this since it is reassigned further down.

--

Up-thread David J had recommended an ordering column (or seqno) that
would provide the order in which the settings were loaded. I think this
would give valuable information that can't be gleaned from the line
numbers alone.

Personally I think it would be fine to start from 1 and increment for
each setting found, rather than rank within a setting. If the user
wants per setting ranking that's what SQL is for. So the output would
look something like:

sourcefile | sourceline | seqno | name | setting
--------------------------+------------+-------+-----------------+-----------
/db/postgresql.conf | 64 | 1 | max_connections | 100
/db/postgresql.conf | 116 | 2 | shared_buffers | 128MB
/db/postgresql.conf | 446 | 3 | log_timezone |
US/Eastern
/db/postgresql.auto.conf | 3 | 4 | max_connections | 200

--
- David Steele
david@pgmasters.net

On Sat, Apr 25, 2015 at 3:40 AM, David Steele <david@pgmasters.net> wrote:

On 4/4/15 9:21 AM, Sawada Masahiko wrote:

I added documentation changes to patch is attached.
Also I tried to use memory context for allocation of guc_file_variable
in TopMemoryContext,
but it was failed access after received SIGHUP.

Below is my review of the v5 patch:

Thank you for your review comment!
The latest patch is attached.

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
+ <sect1 id="view-pg-file-settings">
+  <title><structname>pg_file_settings</structname></title>
+
+  <indexterm zone="view-pg-file-settings">
+   <primary>pg_file_settings</primary>
+  </indexterm>
+
+  <para>
+   The view <structname>pg_file_settings</structname> provides confirm
parameters via SQL,
+   which is written in configuration file. This view can be update by
reloading configration file.
+  </para>

Perhaps something like:

<para>
The view <structname>pg_file_settings</structname> provides access to
run-time parameters
that are defined in configuration files via SQL. In contrast to
<structname>pg_settings</structname> a row is provided for each
occurrence
of the parameter in a configuration file. This is helpful for
discovering why one value
may have been used in preference to another when the parameters were
loaded.
</para>

+  <table>
+   <title><structname>pg_file_settings</> Columns</title>
+
+  <tgroup cols="3">
+   <thead>
+    <row>
+     <entry>Name</entry>
+     <entry>Type</entry>
+     <entry>Description</entry>
+    </row>
+   </thead>
+   <tbody>
+    <row>
+     <entry><structfield>sourcefile</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>A path of configration file</entry>

From pg_settings:

<entry>Configuration file the current value was set in (null for
values set from sources other than configuration files, or when
examined by a non-superuser);
helpful when using <literal>include</> directives in configuration
files</entry>

+    </row>
+    <row>
+     <entry><structfield>sourceline</structfield></entry>
+     <entry><structfield>integer</structfield></entry>
+     <entry>The line number in configuration file</entry>

From pg_settings:

<entry>Line number within the configuration file the current value was
set at (null for values set from sources other than configuration
files,
or when examined by a non-superuser)
</entry>

+    </row>
+    <row>
+     <entry><structfield>name</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>Parameter name in configuration file</entry>

From pg_settings:

<entry>Run-time configuration parameter name</entry>

+    </row>
+    <row>
+     <entry><structfield>setting</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>Value of the parameter in configuration file</entry>
+    </row>
+   </tbody>
+  </tgroup>
+ </table>
+
+</sect1>

The documentation is updated based on your suggestion.

diff --git a/src/backend/utils/misc/guc-file.l
b/src/backend/utils/misc/guc-file.l
@@ -342,6 +345,42 @@ ProcessConfigFile(GucContext context)
+        guc_array = guc_file_variables;
+        for (item = head; item; item = item->next, guc_array++)
+        {
+            free(guc_array->name);
+            free(guc_array->value);
+            free(guc_array->filename);

There's an issue freeing these when calling pg_reload_conf():

Fix.

postgres=# alter system set max_connections = 100;
ALTER SYSTEM
postgres=# select * from pg_reload_conf();
LOG: received SIGHUP, reloading configuration files
pg_reload_conf
----------------
t
(1 row)

postgres=# postgres(25078,0x7fff747b2300) malloc: *** error for object
0x424d380044: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

Of course a config reload can't change max_connections, but I wouldn't
expect it to crash, either.

+ guc_array->sourceline = -1;

I can't see the need for this since it is reassigned further down.

Fix.

--

Up-thread David J had recommended an ordering column (or seqno) that
would provide the order in which the settings were loaded. I think this
would give valuable information that can't be gleaned from the line
numbers alone.

Personally I think it would be fine to start from 1 and increment for
each setting found, rather than rank within a setting. If the user
wants per setting ranking that's what SQL is for. So the output would
look something like:

sourcefile | sourceline | seqno | name | setting
--------------------------+------------+-------+-----------------+-----------
/db/postgresql.conf | 64 | 1 | max_connections | 100
/db/postgresql.conf | 116 | 2 | shared_buffers | 128MB
/db/postgresql.conf | 446 | 3 | log_timezone |
US/Eastern
/db/postgresql.auto.conf | 3 | 4 | max_connections | 200

Yep, I agree with you.
Added seqno column into pg_file_settings view.

Regards,

-------
Sawada Masahiko

On Mon, Apr 27, 2015 at 11:31 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:

On Sat, Apr 25, 2015 at 3:40 AM, David Steele <david@pgmasters.net> wrote:

On 4/4/15 9:21 AM, Sawada Masahiko wrote:

I added documentation changes to patch is attached.
Also I tried to use memory context for allocation of guc_file_variable
in TopMemoryContext,
but it was failed access after received SIGHUP.

Below is my review of the v5 patch:

Thank you for your review comment!
The latest patch is attached.

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
+ <sect1 id="view-pg-file-settings">
+  <title><structname>pg_file_settings</structname></title>
+
+  <indexterm zone="view-pg-file-settings">
+   <primary>pg_file_settings</primary>
+  </indexterm>
+
+  <para>
+   The view <structname>pg_file_settings</structname> provides confirm
parameters via SQL,
+   which is written in configuration file. This view can be update by
reloading configration file.
+  </para>

Perhaps something like:

<para>
The view <structname>pg_file_settings</structname> provides access to
run-time parameters
that are defined in configuration files via SQL. In contrast to
<structname>pg_settings</structname> a row is provided for each
occurrence
of the parameter in a configuration file. This is helpful for
discovering why one value
may have been used in preference to another when the parameters were
loaded.
</para>

+  <table>
+   <title><structname>pg_file_settings</> Columns</title>
+
+  <tgroup cols="3">
+   <thead>
+    <row>
+     <entry>Name</entry>
+     <entry>Type</entry>
+     <entry>Description</entry>
+    </row>
+   </thead>
+   <tbody>
+    <row>
+     <entry><structfield>sourcefile</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>A path of configration file</entry>

From pg_settings:

<entry>Configuration file the current value was set in (null for
values set from sources other than configuration files, or when
examined by a non-superuser);
helpful when using <literal>include</> directives in configuration
files</entry>

+    </row>
+    <row>
+     <entry><structfield>sourceline</structfield></entry>
+     <entry><structfield>integer</structfield></entry>
+     <entry>The line number in configuration file</entry>

From pg_settings:

<entry>Line number within the configuration file the current value was
set at (null for values set from sources other than configuration
files,
or when examined by a non-superuser)
</entry>

+    </row>
+    <row>
+     <entry><structfield>name</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>Parameter name in configuration file</entry>

From pg_settings:

<entry>Run-time configuration parameter name</entry>

+    </row>
+    <row>
+     <entry><structfield>setting</structfield></entry>
+     <entry><structfield>text</structfield></entry>
+     <entry>Value of the parameter in configuration file</entry>
+    </row>
+   </tbody>
+  </tgroup>
+ </table>
+
+</sect1>

The documentation is updated based on your suggestion.

diff --git a/src/backend/utils/misc/guc-file.l
b/src/backend/utils/misc/guc-file.l
@@ -342,6 +345,42 @@ ProcessConfigFile(GucContext context)
+        guc_array = guc_file_variables;
+        for (item = head; item; item = item->next, guc_array++)
+        {
+            free(guc_array->name);
+            free(guc_array->value);
+            free(guc_array->filename);

There's an issue freeing these when calling pg_reload_conf():

Fix.

postgres=# alter system set max_connections = 100;
ALTER SYSTEM
postgres=# select * from pg_reload_conf();
LOG: received SIGHUP, reloading configuration files
pg_reload_conf
----------------
t
(1 row)

postgres=# postgres(25078,0x7fff747b2300) malloc: *** error for object
0x424d380044: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

Of course a config reload can't change max_connections, but I wouldn't
expect it to crash, either.

+ guc_array->sourceline = -1;

I can't see the need for this since it is reassigned further down.

Fix.

--

Up-thread David J had recommended an ordering column (or seqno) that
would provide the order in which the settings were loaded. I think this
would give valuable information that can't be gleaned from the line
numbers alone.

Personally I think it would be fine to start from 1 and increment for
each setting found, rather than rank within a setting. If the user
wants per setting ranking that's what SQL is for. So the output would
look something like:

sourcefile | sourceline | seqno | name | setting
--------------------------+------------+-------+-----------------+-----------
/db/postgresql.conf | 64 | 1 | max_connections | 100
/db/postgresql.conf | 116 | 2 | shared_buffers | 128MB
/db/postgresql.conf | 446 | 3 | log_timezone |
US/Eastern
/db/postgresql.auto.conf | 3 | 4 | max_connections | 200

Yep, I agree with you.
Added seqno column into pg_file_settings view.

Attached patch is modified to apply to HEAD.
v7 patch is latest.

Regards,

-------
Sawada Masahiko

On 4/27/15 10:31 AM, Sawada Masahiko wrote:

Thank you for your review comment!
The latest patch is attached.

Looks good overall - a few more comments below:

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
+    <row>
+     <entry><structfield>seqno</structfield></entry>
+     <entry><structfield>integer</structfield></entry>
+     <entry>Sequence number of current view</entry>

I would suggest:

Order in which the setting was loaded from the configuration

+    </row>
+    <row>
+     <entry><structfield>name</structfield></entry>
+     <entry><structfield>text</structfield></entry>

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
+/*
+ * show_all_file_settings
+ */
+
+#define NUM_PG_FILE_SETTINGS_ATTS 5
+
+Datum
+show_all_file_settings(PG_FUNCTION_ARGS)

It would be good to get more detail in the function comment, as well as
more comments in the function itself.

A minor thing, but there are a number of whitespace errors when applying
the patch:

../000_pg_file_settings_view_v6.patch:159: indent with spaces.
free(guc_file_variables[i].name);
../000_pg_file_settings_view_v6.patch:160: indent with spaces.
free(guc_file_variables[i].value);
../000_pg_file_settings_view_v6.patch:161: indent with spaces.
free(guc_file_variables[i].filename);
../000_pg_file_settings_view_v6.patch:178: indent with spaces.
guc_array->name = guc_strdup(FATAL, item->name);
../000_pg_file_settings_view_v6.patch:179: indent with spaces.
guc_array->value = guc_strdup(FATAL, item->value);
warning: squelched 2 whitespace errors
warning: 7 lines add whitespace errors.

I'm sure the committer would appreciate it if you'd clean those up.

--
- David Steele
david@pgmasters.net

On Tue, Apr 28, 2015 at 3:22 AM, David Steele <david@pgmasters.net> wrote:

On 4/27/15 10:31 AM, Sawada Masahiko wrote:

Thank you for your review comment!
The latest patch is attached.

Looks good overall - a few more comments below:

Thank you for your reviewing.
Attached v8 patch is latest version.

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
+    <row>
+     <entry><structfield>seqno</structfield></entry>
+     <entry><structfield>integer</structfield></entry>
+     <entry>Sequence number of current view</entry>

I would suggest:

Order in which the setting was loaded from the configuration

FIx.

+    </row>
+    <row>
+     <entry><structfield>name</structfield></entry>
+     <entry><structfield>text</structfield></entry>

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
+/*
+ * show_all_file_settings
+ */
+
+#define NUM_PG_FILE_SETTINGS_ATTS 5
+
+Datum
+show_all_file_settings(PG_FUNCTION_ARGS)

It would be good to get more detail in the function comment, as well as
more comments in the function itself.

Added some comments.

A minor thing, but there are a number of whitespace errors when applying
the patch:

../000_pg_file_settings_view_v6.patch:159: indent with spaces.
free(guc_file_variables[i].name);
../000_pg_file_settings_view_v6.patch:160: indent with spaces.
free(guc_file_variables[i].value);
../000_pg_file_settings_view_v6.patch:161: indent with spaces.
free(guc_file_variables[i].filename);
../000_pg_file_settings_view_v6.patch:178: indent with spaces.
guc_array->name = guc_strdup(FATAL, item->name);
../000_pg_file_settings_view_v6.patch:179: indent with spaces.
guc_array->value = guc_strdup(FATAL, item->value);
warning: squelched 2 whitespace errors
warning: 7 lines add whitespace errors.

I'm sure the committer would appreciate it if you'd clean those up.

I tried to get rid of white space.

Regards,

-------
Sawada Masahiko

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Looks good overall, but make installcheck-world does not pass. rules.sql outputs all system views to pg_file_settings will need to be added:

*** src/src/test/regress/expected/rules.out	2015-04-24 12:11:15.000000000 -0400
--- src/src/test/regress/results/rules.out	2015-04-28 10:44:59.000000000 -0400
***************
*** 1308,1313 ****
--- 1308,1319 ----
      c.is_scrollable,
      c.creation_time
     FROM pg_cursor() c(name, statement, is_holdable, is_binary, is_scrollable, creation_time);
+ pg_file_settings| SELECT a.sourcefile,
+     a.sourceline,
+     a.seqno,
+     a.name,
+     a.setting
+    FROM pg_show_all_file_settings() a(sourcefile, sourceline, seqno, name, setting);
  pg_group| SELECT pg_authid.rolname AS groname,
      pg_authid.oid AS grosysid,
      ARRAY( SELECT pg_auth_members.member

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

On 4/27/15 10:37 PM, Sawada Masahiko wrote:

Thank you for your reviewing.
Attached v8 patch is latest version.

I posted a review through the CF app but it only went to the list
instead of recipients of the latest message. install-checkworld is
failing but the fix is pretty trivial.

See:
/messages/by-id/20150428145626.2632.75287.pgcf@coridan.postgresql.org

--
- David Steele
david@pgmasters.net

On Wed, Apr 29, 2015 at 12:20 AM, David Steele <david@pgmasters.net> wrote:

On 4/27/15 10:37 PM, Sawada Masahiko wrote:

Thank you for your reviewing.
Attached v8 patch is latest version.

I posted a review through the CF app but it only went to the list
instead of recipients of the latest message. install-checkworld is
failing but the fix is pretty trivial.

See:
/messages/by-id/20150428145626.2632.75287.pgcf@coridan.postgresql.org

Thank you for reviewing!
I have fixed regarding regression test.
The latest patch is attached.

Regards,

-------
Sawada Masahiko

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Looks good - ready for committer.

The new status of this patch is: Ready for Committer

--
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, Apr 24, 2015 at 2:40 PM, David Steele <david@pgmasters.net> wrote:

<para>
The view <structname>pg_file_settings</structname> provides access to
run-time parameters
that are defined in configuration files via SQL. In contrast to
<structname>pg_settings</structname> a row is provided for each
occurrence
of the parameter in a configuration file. This is helpful for
discovering why one value
may have been used in preference to another when the parameters were
loaded.
</para>

This seems to imply that this gives information about only a subset of
configuration files; specifically, those auto-generated based on SQL
commands - i.e. postgresql.conf.auto. But I think it's supposed to
give information about all configuration files, regardless of how
generated. Am I wrong? If not, I'd suggest "run-time parameters that
are defined in configuration files via SQL" -> "run-time parameters
stored in configuration files".

--
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 4/29/15 5:16 PM, Robert Haas wrote:

On Fri, Apr 24, 2015 at 2:40 PM, David Steele <david@pgmasters.net> wrote:

<para>
The view <structname>pg_file_settings</structname> provides access to
run-time parameters
that are defined in configuration files via SQL. In contrast to
<structname>pg_settings</structname> a row is provided for each
occurrence
of the parameter in a configuration file. This is helpful for
discovering why one value
may have been used in preference to another when the parameters were
loaded.
</para>

This seems to imply that this gives information about only a subset of
configuration files; specifically, those auto-generated based on SQL
commands - i.e. postgresql.conf.auto. But I think it's supposed to
give information about all configuration files, regardless of how
generated. Am I wrong? If not, I'd suggest "run-time parameters that
are defined in configuration files via SQL" -> "run-time parameters
stored in configuration files".

The view does give information about all configuration files regardless
of how they were created.

That's what I intended the text to say but I think your phrasing is clearer.

--
- David Steele
david@pgmasters.net

On Thu, Apr 30, 2015 at 6:31 AM, David Steele <david@pgmasters.net> wrote:

On 4/29/15 5:16 PM, Robert Haas wrote:

On Fri, Apr 24, 2015 at 2:40 PM, David Steele <david@pgmasters.net> wrote:

<para>
The view <structname>pg_file_settings</structname> provides access to
run-time parameters
that are defined in configuration files via SQL. In contrast to
<structname>pg_settings</structname> a row is provided for each
occurrence
of the parameter in a configuration file. This is helpful for
discovering why one value
may have been used in preference to another when the parameters were
loaded.
</para>

This seems to imply that this gives information about only a subset of
configuration files; specifically, those auto-generated based on SQL
commands - i.e. postgresql.conf.auto. But I think it's supposed to
give information about all configuration files, regardless of how
generated. Am I wrong? If not, I'd suggest "run-time parameters that
are defined in configuration files via SQL" -> "run-time parameters
stored in configuration files".

The view does give information about all configuration files regardless
of how they were created.

That's what I intended the text to say but I think your phrasing is clearer.

Thank you for reviewing.
I agree with this.
Attached patch is updated version v10.

Regards,

-------
Sawada Masahiko

Sawada,

* Sawada Masahiko (sawada.mshk@gmail.com) wrote:

Thank you for reviewing.
I agree with this.
Attached patch is updated version v10.

Committed with quite a few additional changes and improvements. Please
take a look, test, and let me know if you see any issues or have any
concerns.

Thanks!

Stephen