Обсуждение: Proposal: knowing detail of config files via SQL
Hi, As per discussion <http://www.postgresql.org/message-id/CAD21AoDkds8Oqbr199wwrCp7fiDvOw6bbb+CGdwQHUF+gX_bVg@mail.gmail.com>, 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. Regards, ------- Sawada Masahiko
On 1/22/15 11:13 AM, Sawada Masahiko wrote: > Hi, > > As per discussion > <http://www.postgresql.org/message-id/CAD21AoDkds8Oqbr199wwrCp7fiDvOw6bbb+CGdwQHUF+gX_bVg@mail.gmail.com>, > 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 actuallyin 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_fileand 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
Sawada Masahiko <sawada.mshk@gmail.com> writes: > As per discussion > <http://www.postgresql.org/message-id/CAD21AoDkds8Oqbr199wwrCp7fiDvOw6bbb+CGdwQHUF+gX_bVg@mail.gmail.com>, > 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
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.
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
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
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
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?
> 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?
>
> 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 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.)
> 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.
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
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
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
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
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
On Fri, Jan 30, 2015 at 9:50 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
pg_file_settings view also has source file column same as pg_settings.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?
>
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
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
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
Masuhiko-san,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
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
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
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
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
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
* 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
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
* 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
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.
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".
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.
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: http://www.postgresql.org/message-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: > > http://www.postgresql.org/message-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: >> >> http://www.postgresql.org/message-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
configurationfiles, 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
sourcesother 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 filespg_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 willneed 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.rolnameAS groname, pg_authid.oid AS grosysid, ARRAY( SELECT pg_auth_members.member
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: http://www.postgresql.org/message-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: > http://www.postgresql.org/message-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
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
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