Re: [HACKERS] Proposal: knowing detail of config files via SQL
Sawada, * Sawada Masahiko (sawada.m...@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 signature.asc Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Wed, Apr 29, 2015 at 12:20 AM, David Steele da...@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.p...@coridan.postgresql.org Thank you for reviewing! I have fixed regarding regression test. The latest patch is attached. Regards, --- Sawada Masahiko 000_pg_file_settings_view_v9.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Looks good - ready for committer. The new status of this patch is: Ready for Committer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Fri, Apr 24, 2015 at 2:40 PM, David Steele da...@pgmasters.net wrote: para The view structnamepg_file_settings/structname provides access to run-time parameters that are defined in configuration files via SQL. In contrast to structnamepg_settings/structname a row is provided for each occurrence of the parameter in a configuration file. This is helpful for discovering why one value may have been used in preference to another when the parameters were loaded. /para This seems to imply that this gives information about only a subset of configuration files; specifically, those auto-generated based on SQL commands - i.e. postgresql.conf.auto. But I think it's supposed to give information about all configuration files, regardless of how generated. Am I wrong? If not, I'd suggest run-time parameters that are defined in configuration files via SQL - run-time parameters stored in configuration files. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 4/29/15 5:16 PM, Robert Haas wrote: On Fri, Apr 24, 2015 at 2:40 PM, David Steele da...@pgmasters.net wrote: para The view structnamepg_file_settings/structname provides access to run-time parameters that are defined in configuration files via SQL. In contrast to structnamepg_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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Thu, Apr 30, 2015 at 6:31 AM, David Steele da...@pgmasters.net wrote: On 4/29/15 5:16 PM, Robert Haas wrote: On Fri, Apr 24, 2015 at 2:40 PM, David Steele da...@pgmasters.net wrote: para The view structnamepg_file_settings/structname provides access to run-time parameters that are defined in configuration files via SQL. In contrast to structnamepg_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 diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 4b79958..adb8628 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -7560,6 +7560,11 @@ /row row + entrylink linkend=view-pg-file-settingsstructnamepg_file_settings/structname/link/entry + entryparameter settings of file/entry + /row + + row entrylink linkend=view-pg-shadowstructnamepg_shadow/structname/link/entry entrydatabase users/entry /row @@ -9173,6 +9178,74 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx /sect1 + sect1 id=view-pg-file-settings + titlestructnamepg_file_settings/structname/title + + indexterm zone=view-pg-file-settings + primarypg_file_settings/primary + /indexterm + + para + The view structnamepg_file_settings/structname provides access to + run-time parameters stored in configuration files. + In contrast to structnamepg_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 + titlestructnamepg_file_settings/ Columns/title + + tgroup cols=3 + thead +row + entryName/entry + entryType/entry + entryDescription/entry +/row + /thead + tbody +row + entrystructfieldsourcefile/structfield/entry + entrystructfieldtext/structfield/entry + entryA path of configration file/entry +/row +row + entrystructfieldsourceline/structfield/entry + entrystructfieldinteger/structfield/entry + 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 + entrystructfieldseqno/structfield/entry + entrystructfieldinteger/structfield/entry + entryOrder in which the setting was loaded from the configuration/entry +/row +row + entrystructfieldname/structfield/entry + entrystructfieldtext/structfield/entry + 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 literalinclude/literal directives in + configuration files + /entry +/row +row + entrystructfieldsetting/structfield/entry + entrystructfieldtext/structfield/entry + entryRun-time configuration parameter name/entry +/row + /tbody + /tgroup + /table + +/sect1 + sect1 id=view-pg-shadow titlestructnamepg_shadow/structname/title diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 2ad01f4..18921c4 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -411,6 +411,12 @@ 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; +REVOKE EXECUTE ON FUNCTION pg_show_all_file_settings() FROM PUBLIC; + CREATE VIEW pg_timezone_abbrevs AS SELECT * FROM pg_timezone_abbrevs(); diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index c5e0fac..873d950 100644 --- a/src/backend/utils/misc/guc-file.l +++ b/src/backend/utils/misc/guc-file.l @@ -119,6 +119,8 @@ ProcessConfigFile(GucContext context) ConfigVariable *item, *head,
Re: [HACKERS] Proposal: knowing detail of config files via SQL
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.p...@coridan.postgresql.org -- - David Steele da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Looks good overall, but make installcheck-world does not pass. rules.sql outputs all system views to pg_file_settings will need to be added: *** src/src/test/regress/expected/rules.out 2015-04-24 12:11:15.0 -0400 --- src/src/test/regress/results/rules.out 2015-04-28 10:44:59.0 -0400 *** *** 1308,1313 --- 1308,1319 c.is_scrollable, c.creation_time FROM pg_cursor() c(name, statement, is_holdable, is_binary, is_scrollable, creation_time); + pg_file_settings| SELECT a.sourcefile, + a.sourceline, + a.seqno, + a.name, + a.setting +FROM pg_show_all_file_settings() a(sourcefile, sourceline, seqno, name, setting); pg_group| SELECT pg_authid.rolname AS groname, pg_authid.oid AS grosysid, ARRAY( SELECT pg_auth_members.member -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Apr 25, 2015 at 3:40 AM, David Steele da...@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
+ titlestructnamepg_file_settings/structname/title
+
+ indexterm zone=view-pg-file-settings
+ primarypg_file_settings/primary
+ /indexterm
+
+ para
+ The view structnamepg_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 structnamepg_file_settings/structname provides access to
run-time parameters
that are defined in configuration files via SQL. In contrast to
structnamepg_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
+ titlestructnamepg_file_settings/ Columns/title
+
+ tgroup cols=3
+ thead
+row
+ entryName/entry
+ entryType/entry
+ entryDescription/entry
+/row
+ /thead
+ tbody
+row
+ entrystructfieldsourcefile/structfield/entry
+ entrystructfieldtext/structfield/entry
+ entryA path of configration file/entry
From pg_settings:
entryConfiguration 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 literalinclude/ directives in configuration
files/entry
+/row
+row
+ entrystructfieldsourceline/structfield/entry
+ entrystructfieldinteger/structfield/entry
+ entryThe line number in configuration file/entry
From pg_settings:
entryLine 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
+ entrystructfieldname/structfield/entry
+ entrystructfieldtext/structfield/entry
+ entryParameter name in configuration file/entry
From pg_settings:
entryRun-time configuration parameter name/entry
+/row
+row
+ entrystructfieldsetting/structfield/entry
+ entrystructfieldtext/structfield/entry
+ entryValue 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
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Tue, Apr 28, 2015 at 3:22 AM, David Steele da...@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 + entrystructfieldseqno/structfield/entry + entrystructfieldinteger/structfield/entry + entrySequence number of current view/entry I would suggest: Order in which the setting was loaded from the configuration FIx. +/row +row + entrystructfieldname/structfield/entry + entrystructfieldtext/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 diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 898865e..50b93cf 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -7437,6 +7437,11 @@ /row row + entrylink linkend=view-pg-file-settingsstructnamepg_file_settings/structname/link/entry + entryparameter settings of file/entry + /row + + row entrylink linkend=view-pg-shadowstructnamepg_shadow/structname/link/entry entrydatabase users/entry /row @@ -9050,6 +9055,74 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx /sect1 + sect1 id=view-pg-file-settings + titlestructnamepg_file_settings/structname/title + + indexterm zone=view-pg-file-settings + primarypg_file_settings/primary + /indexterm + + para + The view structnamepg_file_settings/structname provides access to + run-time parameters that are defined in configuration files via SQL. + In contrast to structnamepg_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 + titlestructnamepg_file_settings/ Columns/title + + tgroup cols=3 + thead +row + entryName/entry + entryType/entry + entryDescription/entry +/row + /thead + tbody +row + entrystructfieldsourcefile/structfield/entry + entrystructfieldtext/structfield/entry + entryA path of configration file/entry +/row +row + entrystructfieldsourceline/structfield/entry + entrystructfieldinteger/structfield/entry + 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 + entrystructfieldseqno/structfield/entry + entrystructfieldinteger/structfield/entry + entryOrder in which the setting was loaded from the configuration/entry +/row +row + entrystructfieldname/structfield/entry + entrystructfieldtext/structfield/entry + 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 literalinclude/literal directives in + configuration files + /entry +/row +row + entrystructfieldsetting/structfield/entry + entrystructfieldtext/structfield/entry + entryRun-time configuration parameter name/entry +/row + /tbody + /tgroup + /table + +/sect1 + sect1 id=view-pg-shadow titlestructnamepg_shadow/structname/title diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 4c35ef4..31faab0 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -411,6 +411,12 @@ CREATE
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Mon, Apr 27, 2015 at 11:31 PM, Sawada Masahiko sawada.m...@gmail.com wrote:
On Sat, Apr 25, 2015 at 3:40 AM, David Steele da...@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
+ titlestructnamepg_file_settings/structname/title
+
+ indexterm zone=view-pg-file-settings
+ primarypg_file_settings/primary
+ /indexterm
+
+ para
+ The view structnamepg_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 structnamepg_file_settings/structname provides access to
run-time parameters
that are defined in configuration files via SQL. In contrast to
structnamepg_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
+ titlestructnamepg_file_settings/ Columns/title
+
+ tgroup cols=3
+ thead
+row
+ entryName/entry
+ entryType/entry
+ entryDescription/entry
+/row
+ /thead
+ tbody
+row
+ entrystructfieldsourcefile/structfield/entry
+ entrystructfieldtext/structfield/entry
+ entryA path of configration file/entry
From pg_settings:
entryConfiguration 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 literalinclude/ directives in configuration
files/entry
+/row
+row
+ entrystructfieldsourceline/structfield/entry
+ entrystructfieldinteger/structfield/entry
+ entryThe line number in configuration file/entry
From pg_settings:
entryLine 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
+ entrystructfieldname/structfield/entry
+ entrystructfieldtext/structfield/entry
+ entryParameter name in configuration file/entry
From pg_settings:
entryRun-time configuration parameter name/entry
+/row
+row
+ entrystructfieldsetting/structfield/entry
+ entrystructfieldtext/structfield/entry
+ entryValue 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 |
Re: [HACKERS] Proposal: knowing detail of config files via SQL
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 + entrystructfieldseqno/structfield/entry + entrystructfieldinteger/structfield/entry + entrySequence number of current view/entry I would suggest: Order in which the setting was loaded from the configuration +/row +row + entrystructfieldname/structfield/entry + entrystructfieldtext/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 da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Wed, Mar 11, 2015 at 11:46 PM, Sawada Masahiko sawada.m...@gmail.com wrote:
On Tue, Mar 10, 2015 at 12:08 PM, Stephen Frost sfr...@snowman.net wrote:
Sawada,
* Sawada Masahiko (sawada.m...@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.go29...@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;
+ charbuffer[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
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Tue, Mar 10, 2015 at 12:08 PM, Stephen Frost sfr...@snowman.net wrote:
Sawada,
* Sawada Masahiko (sawada.m...@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.go29...@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;
+ charbuffer[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
000_pg_file_settings_view_v4.patch
Description: Binary data
--
Sent via
Re: [HACKERS] Proposal: knowing detail of config files via SQL
* Stephen Frost (sfr...@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 signature.asc Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
Sawada,
* Sawada Masahiko (sawada.m...@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.go29...@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;
+ charbuffer[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
signature.asc
Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Feb 28, 2015 at 2:27 PM, Stephen Frost sfr...@snowman.net wrote:
Sawada,
* Sawada Masahiko (sawada.m...@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,
+
Re: [HACKERS] Proposal: knowing detail of config files via SQL
* Peter Eisentraut (pete...@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 signature.asc Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
* Peter Eisentraut (pete...@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 signature.asc Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 3/3/15 5:58 PM, Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: It's not a documented policy but it's certainly what a whole slew of functions *do*. Including pg_start_backup, pg_stop_backup, pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point, pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume, pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir. Indeed, it's the larger portion of what the additional role attributes are for. I'm not entirely thrilled to hear that nearly all of that work might be moot, but if that's the case then so be it and let's just remove all those superuser checks and instead revoke EXECUTE from public and let the superuser grant out those rights. It does seem like that could be an easier and more flexible answer than inventing a pile of role attributes. One issue is that privilege changes on built-in stuff (and extensions) are not dumped. But that is fixable. One aspect of this that merits some thought is that in some cases access to some set of functions is best granted as a unit. That's easy with role properties but much less so with plain GRANT. Do we have enough such cases to make it an issue? You could have built-in roles, such as backup and ship the system with the backup role having permissions on some functions. And then users are granted those roles. Similar to how some Linux systems ship with groups such as adm. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 3/3/15 5:29 PM, Stephen Frost wrote: For my part, I understood that we specifically didn't want to allow that for the same reason that we didn't want to simply depend on the GRANT system for the above functions, but everyone else on these discussions so far is advocating for using the GRANT system. My memory might be wrong, but I could have sworn that I had brought up exactly that suggestion in years past only to have it shot down. I think a lot of this access control work is done based on some undocumented understandings, when in fact there is no consensus on anything. I think we need more clarity. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 3/2/15 4:47 PM, Robert Haas wrote: On Sat, Feb 28, 2015 at 12:27 AM, Stephen Frost sfr...@snowman.net wrote: While this generally works, the usual expectation is that functions that should be superuser-only have a check in the function rather than depending on the execute privilege. I'm certainly happy to debate the merits of that approach, but for the purposes of this patch, I'd suggest you stick an if (!superuser()) ereport(must be superuser) into the function itself and not work about setting the correct permissions on it. -1. If that policy exists at all, it's a BAD policy, because it prevents users from changing the permissions using DDL. I think the superuser check should be inside the function, when, for example, it masks some of the output data depending on the user's permissions. But I see little virtue in handicapping an attempt by a superuser to grant SELECT rights on pg_file_settings. This is in fact how most if not all code ensures supervisor-only access to functions, so for the purpose of this patch, I think it is the correct approach. Someone may well change that soon after, if the other ongoing efforts conclude. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
* Robert Haas (robertmh...@gmail.com) wrote: On Sat, Feb 28, 2015 at 12:27 AM, Stephen Frost sfr...@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 signature.asc Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
Jim, * Jim Nasby (jim.na...@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 signature.asc Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Tue, Mar 3, 2015 at 10:29 PM, Stephen Frost sfr...@snowman.net wrote: -1. If that policy exists at all, it's a BAD policy, because it prevents users from changing the permissions using DDL. I think the superuser check should be inside the function, when, for example, it masks some of the output data depending on the user's permissions. But I see little virtue in handicapping an attempt by a superuser to grant SELECT rights on pg_file_settings. It's not a documented policy but it's certainly what a whole slew of functions *do*. Including pg_start_backup, pg_stop_backup, pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point, pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume, pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir. And the same issue comes up for the pg_hba_settings patch. Fwiw it's not entirely true that users can't change the permissions on these functions via DDL -- it's just a lot harder. They have to create a security-definer wrapper function and then grant access to that function to who they want. I'm generally of the opinion we should use the GRANT system and not hard-wire things but I also see the concern that it's really easy to grant access to something without realizing all the consequences. It's a lot harder to audit your system to be sure nothing inappropriate has been granted which can be escalated to super-user access. It's not clear how to determine what the set of things are that could do that. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
Stephen Frost sfr...@snowman.net writes: It's not a documented policy but it's certainly what a whole slew of functions *do*. Including pg_start_backup, pg_stop_backup, pg_switch_xlog, pg_rotate_logfile, pg_create_restore_point, pg_xlog_replay_pause, lo_import, lo_export, and pg_xlog_replay_resume, pg_read_file, pg_read_text_file, pg_read_binary_file, and pg_ls_dir. Indeed, it's the larger portion of what the additional role attributes are for. I'm not entirely thrilled to hear that nearly all of that work might be moot, but if that's the case then so be it and let's just remove all those superuser checks and instead revoke EXECUTE from public and let the superuser grant out those rights. It does seem like that could be an easier and more flexible answer than inventing a pile of role attributes. The discussion I'm having with Peter on another thread is a very similar case that should be looping in, which is if we should continue to have any superuser check on updating catalog tables. He is advocating that we remove that also and let the superuser GRANT modification access on catalog tables to users. For my part, I understood that we specifically didn't want to allow that for the same reason that we didn't want to simply depend on the GRANT system for the above functions, but everyone else on these discussions so far is advocating for using the GRANT system. My memory might be wrong, but I could have sworn that I had brought up exactly that suggestion in years past only to have it shot down. I'm a bit less comfortable with this one, mainly because the ability to modify the system catalogs directly implies the ability to crash, corrupt, or hijack the database in any number of non-obvious ways. I would go so far as to argue that if you grant me modify permissions on many of the catalogs, I would have the ability to become superuser outright, and therefore any notion that such a grant is any weaker than granting full superuserness is a dangerous lie. It may be that the same type of permissions escalation is possible with some of the functions you mention above; but anywhere that it isn't, I should think GRANT on the function is a reasonable solution. One aspect of this that merits some thought is that in some cases access to some set of functions is best granted as a unit. That's easy with role properties but much less so with plain GRANT. Do we have enough such cases to make it an issue? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 3/3/15 5:22 PM, Stephen Frost wrote: The problem with the role attribute approach is that they aren't inheirted the way GRANTs are, which means you can't have a backup role that is then granted out to users, you'd have to set a BACKUP role attribute for every role added. Yeah, but you'd still have to grant backup to every role created anyway, right? Or you could create a role that has the backup attribute and then grant that to users. Then they'd have to intentionally SET ROLE my_backup_role to elevate their privilege. That seems like a safer way to do things... -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@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 signature.asc Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
* Stephen Frost (sfr...@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 signature.asc Description: Digital signature
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Feb 28, 2015 at 12:27 AM, Stephen Frost sfr...@snowman.net wrote: While this generally works, the usual expectation is that functions that should be superuser-only have a check in the function rather than depending on the execute privilege. I'm certainly happy to debate the merits of that approach, but for the purposes of this patch, I'd suggest you stick an if (!superuser()) ereport(must be superuser) into the function itself and not work about setting the correct permissions on it. -1. If that policy exists at all, it's a BAD policy, because it prevents users from changing the permissions using DDL. I think the superuser check should be inside the function, when, for example, it masks some of the output data depending on the user's permissions. But I see little virtue in handicapping an attempt by a superuser to grant SELECT rights on pg_file_settings. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 2/27/15 11:27 PM, Stephen Frost wrote:
@@ -344,6 +346,21 @@ ProcessConfigFile(GucContext context)
PGC_BACKEND,
PGC_S_DYNAMIC_DEFAULT);
}
+ guc_file_variables = (ConfigFileVariable *)
+ guc_malloc(FATAL, num_guc_file_variables * sizeof(struct
ConfigFileVariable));
Uh, perhaps I missed it, but what happens on a reload? Aren't we going
to realloc this every time? Seems like we should be doing a
guc_malloc() the first time through but doing guc_realloc() after, or
we'll leak memory on every reload.
+ /*
+* Apply guc config parameters to guc_file_variable
+*/
+ guc = guc_file_variables;
+ for (item = head; item; item = item-next, guc++)
+ {
+ guc-name = guc_strdup(FATAL, item-name);
+ guc-value = guc_strdup(FATAL, item-value);
+ guc-filename = guc_strdup(FATAL, item-filename);
+ guc-sourceline = item-sourceline;
+ }
Uh, ditto and double-down here. I don't see a great solution other than
looping through the previous array and free'ing each of these, since we
can't depend on the memory context machinery being up and ready at this
point, as I recall.
MemoryContextInit() happens near the top of main(), before we call
InitializeGUCOptions(). So it should be possible to use memory contexts
here. I don't know why guc doesn't use palloc; perhaps for historical
reasons at this point?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
Sawada,
* Sawada Masahiko (sawada.m...@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,
Re: [HACKERS] Proposal: knowing detail of config files via SQL
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.blackw...@rrd.com http://www.rrdonnelley.com http://www.rrdonnelley.com/ * mike.blackw...@rrd.com* On Fri, Jan 30, 2015 at 9:50 AM, Sawada Masahiko sawada.m...@gmail.com wrote: On Sat, Jan 31, 2015 at 12:24 AM, David Fetter da...@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 robertmh...@gmail.com wrote: On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Is that a requirement, and if so why? Because this proposal doesn't guarantee any such knowledge AFAICS. The proposal provides for SQL access to all possible sources of variable value setting and, ideally, a means of ordering them in priority order, so that a search for TimeZone would return two records, one for postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and 2 respectively - so that in looking at that result if the postgresql.auto.conf entry were to be removed the user would know that what the value is in postgresql.conf that would become active. Furthermore, if postgresql.conf has a setting AND there is a mapping in an #included file that information would be accessible via SQL as well. I know what the proposal is. What I am questioning is the use-case that justifies having us build and support all this extra mechanism. How often does anyone need to know what the next down variable value would be? I believe I've run into situations where this would be useful. I wouldn't be willing to put in the effort to do this myself, but I wouldn't be prepared to vote against a high-quality patch that someone else felt motivated to write. Attached patch adds new view pg_file_settings (WIP version). This view behaves like followings, [postgres][5432](1)=# select * from pg_file_settings ; name| setting | sourcefile ++ max_connections| 100| /home/masahiko/pgsql/master/3data/postgresql.conf shared_buffers | 128MB | /home/masahiko/pgsql/master/3data/postgresql.conf This looks great! Is there a reason not to have the sourcefile as a column in pg_settings? pg_file_settings view also has source file column same as pg_settings. It might was hard to confirm that column in previous mail. I put example of pg_file_settings again as follows. [postgres][5432](1)=# select * from pg_file_settings where name = 'work_mem'; -[ RECORD 1 ]-- name | work_mem setting| 128MB sourcefile | /home/masahiko/pgsql/master/3data/hoge.conf -[ RECORD 2 ]-- name | work_mem setting| 64MB sourcefile | /home/masahiko/pgsql/master/3data/postgresql.auto.conf Regards, --- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Fri, Jan 30, 2015 at 09:38:10PM +0900, Sawada Masahiko wrote: On Tue, Jan 27, 2015 at 3:34 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane t...@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 da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Jan 31, 2015 at 12:58 AM, Mike Blackwell mike.blackw...@rrd.com wrote: This would default to being available to superusers only, right? Details of the file system shouldn't be available to any random user. This WIP patch does not behave like that, but I agree. This view would be effective combine with ALTER SYSTEM, and ALTER SYSTEM command is executable to superuser only also. Regards, --- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Jan 31, 2015 at 12:24 AM, David Fetter da...@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 robertmh...@gmail.com wrote: On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Is that a requirement, and if so why? Because this proposal doesn't guarantee any such knowledge AFAICS. The proposal provides for SQL access to all possible sources of variable value setting and, ideally, a means of ordering them in priority order, so that a search for TimeZone would return two records, one for postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and 2 respectively - so that in looking at that result if the postgresql.auto.conf entry were to be removed the user would know that what the value is in postgresql.conf that would become active. Furthermore, if postgresql.conf has a setting AND there is a mapping in an #included file that information would be accessible via SQL as well. I know what the proposal is. What I am questioning is the use-case that justifies having us build and support all this extra mechanism. How often does anyone need to know what the next down variable value would be? I believe I've run into situations where this would be useful. I wouldn't be willing to put in the effort to do this myself, but I wouldn't be prepared to vote against a high-quality patch that someone else felt motivated to write. Attached patch adds new view pg_file_settings (WIP version). This view behaves like followings, [postgres][5432](1)=# select * from pg_file_settings ; name| setting | sourcefile ++ max_connections| 100| /home/masahiko/pgsql/master/3data/postgresql.conf shared_buffers | 128MB | /home/masahiko/pgsql/master/3data/postgresql.conf This looks great! Is there a reason not to have the sourcefile as a column in pg_settings? pg_file_settings view also has source file column same as pg_settings. It might was hard to confirm that column in previous mail. I put example of pg_file_settings again as follows. [postgres][5432](1)=# select * from pg_file_settings where name = 'work_mem'; -[ RECORD 1 ]-- name | work_mem setting| 128MB sourcefile | /home/masahiko/pgsql/master/3data/hoge.conf -[ RECORD 2 ]-- name | work_mem setting| 64MB sourcefile | /home/masahiko/pgsql/master/3data/postgresql.auto.conf Regards, --- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
David Johnston david.g.johns...@gmail.com writes: On Fri, Jan 30, 2015 at 12:05 PM, David Fetter da...@fetter.org wrote: Why might the contents of pg_settings be different from what would be in pg_file_settings, apart from the existence of this column? ââThe contents of pg_settings uses the setting name as a primary key. The contents of pg_file_setting uses a compound key consisting of the setting name and the source file. [ raised eyebrow... ] Seems insufficient in the case that multiple settings of the same value occur in the same source file. Don't you need a line number in the key as well? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Fri, Jan 30, 2015 at 12:05 PM, David Fetter da...@fetter.org wrote: On Sat, Jan 31, 2015 at 12:50:20AM +0900, Sawada Masahiko wrote: [postgres][5432](1)=# select * from pg_file_settings where name = 'work_mem'; -[ RECORD 1 ]-- name | work_mem setting| 128MB sourcefile | /home/masahiko/pgsql/master/3data/hoge.conf -[ RECORD 2 ]-- name | work_mem setting| 64MB sourcefile | /home/masahiko/pgsql/master/3data/postgresql.auto.conf Masuhiko-san, I apologize for not communicating clearly. My alternative proposal is to add a NULL-able sourcefile column to pg_settings. This is as an alternative to creating a new pg_file_settings view. Why might the contents of pg_settings be different from what would be in pg_file_settings, apart from the existence of this column? The contents of pg_settings uses the setting name as a primary key. The contents of pg_file_setting uses a compound key consisting of the setting name and the source file. See work_mem in the provided example. More specifically pg_settings only care about the currently active setting while this cares about inactive settings as well. David J.
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Jan 31, 2015 at 12:50:20AM +0900, Sawada Masahiko wrote: On Sat, Jan 31, 2015 at 12:24 AM, David Fetter da...@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 robertmh...@gmail.com wrote: On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane t...@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 da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Jan 31, 2015 at 4:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Fri, Jan 30, 2015 at 12:05 PM, David Fetter da...@fetter.org wrote: Why might the contents of pg_settings be different from what would be in pg_file_settings, apart from the existence of this column? The contents of pg_settings uses the setting name as a primary key. The contents of pg_file_setting uses a compound key consisting of the setting name and the source file. [ raised eyebrow... ] Seems insufficient in the case that multiple settings of the same value occur in the same source file. Don't you need a line number in the key as well? Yep, a line number is also needed. The source file and line number would be primary key of pg_file_settings view. I need to change like that. Regards, --- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Jan 31, 2015 at 3:20 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Sat, Jan 31, 2015 at 2:00 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Sat, Jan 31, 2015 at 4:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Fri, Jan 30, 2015 at 12:05 PM, David Fetter da...@fetter.org wrote: Why might the contents of pg_settings be different from what would be in pg_file_settings, apart from the existence of this column? The contents of pg_settings uses the setting name as a primary key. The contents of pg_file_setting uses a compound key consisting of the setting name and the source file. [ raised eyebrow... ] Seems insufficient in the case that multiple settings of the same value occur in the same source file. Don't you need a line number in the key as well? Yep, a line number is also needed. The source file and line number would be primary key of pg_file_settings view. I need to change like that. Attached patch is latest version including following changes. - This view is available to super use only - Add sourceline coulmn Also I registered CF app. Sorry, I registered unnecessary (extra) threads to CF app. How can I delete them? Regards, --- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Sat, Jan 31, 2015 at 2:00 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Sat, Jan 31, 2015 at 4:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Fri, Jan 30, 2015 at 12:05 PM, David Fetter da...@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 000_pg_file_settings_view_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Tue, Jan 27, 2015 at 3:34 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jan 22, 2015 at 5:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane t...@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 000_pg_file_settings_view_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Fri, Jan 23, 2015 at 4:36 AM, Jim Nasby jim.na...@bluetreble.com wrote: Would this view have a row for every option in a config file? IE: if you set something in both postgresql.conf and postgresql.auto.conf, would it show up twice? I think it should, and that there should be a way to see which setting is actually in effect. yes. It looks like you're attempting to handle #include, yes? Of course yes. Also other idea is to add additional columns existing view (pg_settings), according to prev discussion. I think it would be useful to have a separate view that shows all occurrences of a setting. I recall some comment about source_file and source_line not always being correct in pg_settings; if that's true we should fix that. You mean that pg_settings view shows the variable in current session? pg_settings view shows can be changed if user set the GUC parameter in session or GUC parameter is set to role individually. But I don't think pg_file_settings view has such a behavior. Regards, --- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
Sawada Masahiko sawada.m...@gmail.com writes: As per discussion http://www.postgresql.org/message-id/cad21aodkds8oqbr199wwrcp7fidvow6bbb+cgdwqhuf+gx_...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
Tom Lane-2 wrote regression=# alter system reset timezone; ALTER SYSTEM regression=# select pg_reload_conf(); How does someone know that performing the above commands will result in the TimeZone setting being changed from Asia/Shanghai to US/Eastern? David J. -- View this message in context: http://postgresql.nabble.com/Proposal-knowing-detail-of-config-files-via-SQL-tp5835075p5835142.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: David G Johnston david.g.johns...@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.
Re: [HACKERS] Proposal: knowing detail of config files via SQL
David G Johnston david.g.johns...@gmail.com writes: Tom Lane-2 wrote regression=# alter system reset timezone; ALTER SYSTEM regression=# select pg_reload_conf(); How does someone know that performing the above commands will result in the TimeZone setting being changed from Asia/Shanghai to US/Eastern? Is that a requirement, and if so why? Because this proposal doesn't guarantee any such knowledge AFAICS. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
David Johnston david.g.johns...@gmail.com writes: On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane t...@sss.pgh.pa.us wrote: Is that a requirement, and if so why? Because this proposal doesn't guarantee any such knowledge AFAICS. âThe proposal provides for SQL access to all possible sources of variable value setting and, ideally, a means of ordering them in priority order, so that a search for TimeZone would return two records, one for postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and 2 respectively - so that in looking at that result if the postgresql.auto.conf entry were to be removed the user would know that what the value is in postgresql.conf that would become active. Furthermore, if postgresql.conf has a setting AND there is a mapping in an #included file that information would be accessible via SQL as well. I know what the proposal is. What I am questioning is the use-case that justifies having us build and support all this extra mechanism. How often does anyone need to know what the next down variable value would be? And if they do need to know whether a variable is set in postgresql.conf, how often wouldn't they just resort to grep instead? (Among other points, grep would succeed at noticing commented-out entries, which this mechanism would not.) GUC has existed in more or less its current state for about 15 years, and I don't recall a lot of complaints that would be solved by this. Furthermore, given that ALTER SYSTEM was sold to us as more or less obsoleting manual editing of postgresql.conf, I rather doubt that it's changed the basis of discussion all that much. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Thu, Jan 22, 2015 at 3:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Johnston david.g.johns...@gmail.com writes: On Thu, Jan 22, 2015 at 3:04 PM, Tom Lane t...@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.
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On Fri, Jan 23, 2015 at 3:49 AM, Tom Lane t...@sss.pgh.pa.us wrote: I know what the proposal is. What I am questioning is the use-case that justifies having us build and support all this extra mechanism. How often does anyone need to know what the next down variable value would be? I would say not that often as I think nobody changes the settings in configuration files every now and then, but it could still be meaningful in situations as described upthread by Sawada. I think similar to this, pg_reload_conf() or many such things will be used not that frequently, it seems to me that here important point to consider is that whether such a view could serve any purpose for real users? If we see multiple value for same config parameter was possible even previous to Alter System and there doesn't seem to be much need/demand for such a view and the reason could be that user has no way of changing the setting at file level with command, but now as we provide a way it could be useful in certain cases when user want to retain previous values (value in postgresql.conf). I understand that usage of such a view is not that high, but it could be meaningful in some cases. And if they do need to know whether a variable is set in postgresql.conf, how often wouldn't they just resort to grep instead? Do always user/dba (having superuser privilege) access to postgresql.conf file? It seems to me even if they have access, getting the information via SQL would be more appealing. (Among other points, grep would succeed at noticing commented-out entries, which this mechanism would not.) GUC has existed in more or less its current state for about 15 years, and I don't recall a lot of complaints that would be solved by this. Furthermore, given that ALTER SYSTEM was sold to us as more or less obsoleting manual editing of postgresql.conf, I rather doubt that it's changed the basis of discussion all that much. By providing such a view I don't mean to recommend the user to change the settings by editing postgresql.conf manually and then use Alter System for other cases, rather it could be used for some cases like for default values or if in rare cases user has changed the parameters manually. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 01/22/2015 02:09 PM, David Johnston wrote: The proposal provides for SQL access to all possible sources of variable value setting and, ideally, a means of ordering them in priority order, so that a search for TimeZone would return two records, one for postgresql.auto.conf and one for postgresql.conf - which are numbered 1 and 2 respectively - so that in looking at that result if the postgresql.auto.conf entry were to be removed the user would know that what the value is in postgresql.conf that would become active. Furthermore, if postgresql.conf has a setting AND there is a mapping in an #included file that information would be accessible via SQL as well. Wow. Um, I can't imagine any use for that which would justify the overhead. And I'm practically the settings geek. Note that a single file can have multiple copies of the same GUC, plus there's GUCs set interactively, as well as in the user and database properties. So you're looking at a lot of different versions. I think you're in a position of needing to interest someone else in this issue enough to produce a patch to argue about. I'm not seeing a lot of interest in it here. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: knowing detail of config files via SQL
On 1/22/15 11:13 AM, Sawada Masahiko wrote: Hi, As per discussion http://www.postgresql.org/message-id/cad21aodkds8oqbr199wwrcp7fidvow6bbb+cgdwqhuf+gx_...@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 actually in effect. It looks like you're attempting to handle #include, yes? Also other idea is to add additional columns existing view (pg_settings), according to prev discussion. I think it would be useful to have a separate view that shows all occurrences of a setting. I recall some comment about source_file and source_line not always being correct in pg_settings; if that's true we should fix that. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
