Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-19 Thread Haribabu kommi
On 19 November 2013 09:59 Amit Kapila wrote:
 On Mon, Nov 18, 2013 at 8:31 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 18 November 2013 20:01 Amit Kapila wrote:
   Code changes are fine.
   If two or three errors are present in the configuration file, it
  prints the last error
   Configuration parameter file only. Is it required to be mentioned
   in
  the documentation?
 
  Do you mean to say parsing errors or some run-time error, could you
  explain with example?
 
  LOG:  parameter shared_buffers cannot be changed without restarting
  the server
  LOG:  parameter port cannot be changed without restarting the
 server
  LOG:  configuration file
  /home/hari/installation/bin/../../data/postgresql.auto.conf
 contains
  errors; unaffected changes were applied
 
  The shared_buffers parameter is changed in postgresql.conf and port
 is changed in postgresql.auto.conf.
  The error file displays the last error occurred file.
 
This is only possible if user tries to change configuration
 parameters both by Alter System command and manually as well and the
 case above is
not an error, it is just an information for user that there are some
 parameters changed in config file which can only get reflected after
 server restart.
So definitely user can check log files to see which parameter's
 doesn't get reflected if he is expecting some parameter to be changed,
 but it's not
changed. I think here even in logs if last file containing errors is
 mentioned is not a big problem.
 
  Is it required to mention the above behavior in the document?
 
It is better to show in log both the files rather than documenting
 it, if the the above case is helpful for user which I don't think so,
 also to handle case
in code can complicate the error handling path of code a bit. So I
 think we can leave this case as-is.

Ok fine I marked the patch as ready for committer.

Regards,
Hari babu.



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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-19 Thread Amit Kapila
On Tue, Nov 19, 2013 at 2:06 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 19 November 2013 09:59 Amit Kapila wrote:
 On Mon, Nov 18, 2013 at 8:31 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 18 November 2013 20:01 Amit Kapila wrote:
   Code changes are fine.
   If two or three errors are present in the configuration file, it
  prints the last error

 Ok fine I marked the patch as ready for committer.

   Thanks for review.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-18 Thread Haribabu kommi
On 17 November 2013 12:25 Amit Kapila wrote:
 On Sat, Nov 16, 2013 at 4:35 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 16 November 2013 09:43 Amit Kapila wrote:
  On Fri, Nov 15, 2013 at 10:18 PM, Peter Eisentraut pete...@gmx.net
  wrote:
   On 11/14/13, 2:50 AM, Amit Kapila wrote:
   Find the rebased version attached with this mail.
  
  ereport(ERROR,
 
 (errcode(ERRCODE_CONFIG_FILE_ERROR),
   errmsg(configuration file
 \%s\ contains errors,
  -
 ConfigFileName)));
  +
  + ErrorConfFile)));
 
  The ErrorConfFile prints postgresql.auto.conf only if there is any
  parsing problem with postgresql.auto.conf otherwise it always print
 postgresql.conf because of any other error.
 
Changed to ensure ErrorConfFile contains proper config file name.
Note: I have not asssigned file name incase of error in below loop,
 as file name in gconf is NULL in most cases and moreover this loops
 over
 guc_variables which doesn't contain values/parameters from
 auto.conf. So I don't think it is required to assign ErrorConfFile in
 this loop.
 
 ProcessConfigFile(GucContext context)
 {
 ..
for (i = 0; i  num_guc_variables; i++)
{
struct config_generic *gconf = guc_variables[i];
 
 ..
 }

Code changes are fine. 
If two or three errors are present in the configuration file, it prints the 
last error
Configuration parameter file only. Is it required to be mentioned in the 
documentation?

 
  if any postmaster setting which are set by the alter system command
  which leads to failure of server start, what is the solution to user
  to proceed further to start the server. As it is mentioned that the
  auto.conf file shouldn't be edited manually.
 
 Yeah, but in case of emergency user can change it to get server started.
 Now the question is whether to mention it in documentation, I think we
 can leave this decision to committer. If he thinks that it is better to
 document then I will update it.

Ok fine.

Regards,
Hari babu.


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-18 Thread Amit Kapila
On Mon, Nov 18, 2013 at 6:28 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 17 November 2013 12:25 Amit Kapila wrote:
 On Sat, Nov 16, 2013 at 4:35 PM, Haribabu kommi
   Find the rebased version attached with this mail.
  
  ereport(ERROR,
 
 (errcode(ERRCODE_CONFIG_FILE_ERROR),
   errmsg(configuration file
 \%s\ contains errors,
  -
 ConfigFileName)));
  +
  + ErrorConfFile)));
 
  The ErrorConfFile prints postgresql.auto.conf only if there is any
  parsing problem with postgresql.auto.conf otherwise it always print
 postgresql.conf because of any other error.

Changed to ensure ErrorConfFile contains proper config file name.
Note: I have not asssigned file name incase of error in below loop,
 as file name in gconf is NULL in most cases and moreover this loops
 over
 guc_variables which doesn't contain values/parameters from
 auto.conf. So I don't think it is required to assign ErrorConfFile in
 this loop.

 ProcessConfigFile(GucContext context)
 {
 ..
for (i = 0; i  num_guc_variables; i++)
{
struct config_generic *gconf = guc_variables[i];

 ..
 }

 Code changes are fine.
 If two or three errors are present in the configuration file, it prints the 
 last error
 Configuration parameter file only. Is it required to be mentioned in the 
 documentation?

Do you mean to say parsing errors or some run-time error, could you
explain with example?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-18 Thread Haribabu kommi
On 18 November 2013 20:01 Amit Kapila wrote:
 On Mon, Nov 18, 2013 at 6:28 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
  On 17 November 2013 12:25 Amit Kapila wrote:
  On Sat, Nov 16, 2013 at 4:35 PM, Haribabu kommi
Find the rebased version attached with this mail.
   
   ereport(ERROR,
  
  (errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg(configuration
 file
  \%s\ contains errors,
   -
  ConfigFileName)));
   +
   + ErrorConfFile)));
  
   The ErrorConfFile prints postgresql.auto.conf only if there is
   any parsing problem with postgresql.auto.conf otherwise it always
   print
  postgresql.conf because of any other error.
 
 Changed to ensure ErrorConfFile contains proper config file name.
 Note: I have not asssigned file name incase of error in below
  loop, as file name in gconf is NULL in most cases and moreover this
  loops over
  guc_variables which doesn't contain values/parameters
  from auto.conf. So I don't think it is required to assign
  ErrorConfFile in this loop.
 
  ProcessConfigFile(GucContext context) { ..
 for (i = 0; i  num_guc_variables; i++)
 {
 struct config_generic *gconf = guc_variables[i];
 
  ..
  }
 
  Code changes are fine.
  If two or three errors are present in the configuration file, it
 prints the last error
  Configuration parameter file only. Is it required to be mentioned in
 the documentation?
 
 Do you mean to say parsing errors or some run-time error, could you
 explain with example?

LOG:  parameter shared_buffers cannot be changed without restarting the server
LOG:  parameter port cannot be changed without restarting the server
LOG:  configuration file 
/home/hari/installation/bin/../../data/postgresql.auto.conf contains errors; 
unaffected changes were applied

The shared_buffers parameter is changed in postgresql.conf and port is changed 
in postgresql.auto.conf.
The error file displays the last error occurred file.

Is it required to mention the above behavior in the document?

Regards,
Hari babu


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-18 Thread Amit Kapila
On Mon, Nov 18, 2013 at 8:31 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 18 November 2013 20:01 Amit Kapila wrote:
  Code changes are fine.
  If two or three errors are present in the configuration file, it
 prints the last error
  Configuration parameter file only. Is it required to be mentioned in
 the documentation?

 Do you mean to say parsing errors or some run-time error, could you
 explain with example?

 LOG:  parameter shared_buffers cannot be changed without restarting the 
 server
 LOG:  parameter port cannot be changed without restarting the server
 LOG:  configuration file 
 /home/hari/installation/bin/../../data/postgresql.auto.conf contains 
 errors; unaffected changes were applied

 The shared_buffers parameter is changed in postgresql.conf and port is 
 changed in postgresql.auto.conf.
 The error file displays the last error occurred file.

   This is only possible if user tries to change configuration
parameters both by Alter System command and manually as well and the
case above is
   not an error, it is just an information for user that there are
some parameters changed in config file which can only get reflected
after server restart.
   So definitely user can check log files to see which parameter's
doesn't get reflected if he is expecting some parameter to be changed,
but it's not
   changed. I think here even in logs if last file containing errors
is mentioned is not a big problem.

 Is it required to mention the above behavior in the document?

   It is better to show in log both the files rather than documenting
it, if the the above case is helpful for user which I don't think so,
also to handle case
   in code can complicate the error handling path of code a bit. So I
think we can leave this case as-is.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-16 Thread Haribabu kommi

On 16 November 2013 09:43 Amit Kapila wrote:
 On Fri, Nov 15, 2013 at 10:18 PM, Peter Eisentraut pete...@gmx.net
 wrote:
  On 11/14/13, 2:50 AM, Amit Kapila wrote:
  Find the rebased version attached with this mail.
 
  Doesn't build:
 
  openjade  -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D .
 -c /usr/share/sgml/docbook/stylesheet/dsssl/modular/catalog -d
 stylesheet.dsl -t sgml -i output-html -V html-index postgres.sgml
  openjade:reference.sgml:61:3:E: cannot find alter_system.sgml;
 tried ref/alter_system.sgml, ./alter_system.sgml,
 ./alter_system.sgml, /usr/local/share/sgml/alter_system.sgml,
 /usr/share/sgml/alter_system.sgml
  openjade:config.sgml:164:27:X: reference to non-existent ID SQL-
 ALTERSYSTEM
  make[3]: *** [HTML.index] Error 1
  make[3]: *** Deleting file `HTML.index'
  osx -D. -x lower -i include-xslt-index postgres.sgml postgres.xmltmp
  osx:reference.sgml:61:3:E: cannot find alter_system.sgml; tried
 ref/alter_system.sgml, ./alter_system.sgml,
 /usr/local/share/sgml/alter_system.sgml,
 /usr/share/sgml/alter_system.sgml
  osx:config.sgml:164:27:X: reference to non-existent ID SQL-
 ALTERSYSTEM
  make[3]: *** [postgres.xml] Error 1
 
  New file missing in patch?
 
 Oops, missed the new sgml file in patch, updated patch to include it.
 Many thanks for checking it.

1. Patch applied properly
2. No warnings in the compilation
3. Regress test passed.
4. Basic tests are passed.

Please find review comments:

+ * ALTER SYSTEM SET
+ *
+ * Command to edit postgresql.conf
+ */

I feel it should be Command to change the configuration parameter
because this command is not edits the postgresql.conf file.

ereport(ERROR,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
 errmsg(configuration file \%s\ 
contains errors,
-   ConfigFileName)));
+   ErrorConfFile)));

The ErrorConfFile prints postgresql.auto.conf only if there is any parsing 
problem
with postgresql.auto.conf otherwise it always print postgresql.conf because 
of any other error.
 

+ * A stale temporary file may be left behind in case we crash.
+ * Such files are removed on the next server restart.

The above comment is wrong, the stale temporary file will be used
in the next ALTER SYSTEM command. I didn't find any code where it gets
deleted on the next server restart.


if any postmaster setting which are set by the alter system command which 
leads to failure of server start, what is the solution to user to proceed
further to start the server. As it is mentioned that the auto.conf file
shouldn't be edited manually.


Regards,
Hari babu.


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-16 Thread Amit Kapila
On Sat, Nov 16, 2013 at 4:35 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 16 November 2013 09:43 Amit Kapila wrote:
 On Fri, Nov 15, 2013 at 10:18 PM, Peter Eisentraut pete...@gmx.net
 wrote:
  On 11/14/13, 2:50 AM, Amit Kapila wrote:
  Find the rebased version attached with this mail.
 
 Please find review comments:

 + * ALTER SYSTEM SET
 + *
 + * Command to edit postgresql.conf
 + 
 */

 I feel it should be Command to change the configuration parameter
 because this command is not edits the postgresql.conf file.

   Changed the comment, but I think it is better to use persistently
in line suggested by you, so I have done that way.

 ereport(ERROR,
 (errcode(ERRCODE_CONFIG_FILE_ERROR),
  errmsg(configuration file \%s\ 
 contains errors,
 -   ConfigFileName)));
 +   ErrorConfFile)));

 The ErrorConfFile prints postgresql.auto.conf only if there is any parsing 
 problem
 with postgresql.auto.conf otherwise it always print postgresql.conf because 
 of any other error.

   Changed to ensure ErrorConfFile contains proper config file name.
   Note: I have not asssigned file name incase of error in below loop,
as file name in gconf is NULL in most cases and moreover this loops
over
guc_variables which doesn't contain values/parameters from
auto.conf. So I don't think it is required to assign ErrorConfFile in
this loop.

ProcessConfigFile(GucContext context)
{
..
   for (i = 0; i  num_guc_variables; i++)
   {
   struct config_generic *gconf = guc_variables[i];

..
}


 + * A stale temporary file may be left behind in case we crash.
 + * Such files are removed on the next server restart.

 The above comment is wrong, the stale temporary file will be used
 in the next ALTER SYSTEM command. I didn't find any code where it gets
 deleted on the next server restart.

   Removed the comment from top of function and modified the comment
where file is getting opened.


 if any postmaster setting which are set by the alter system command which
 leads to failure of server start, what is the solution to user to proceed
 further to start the server. As it is mentioned that the auto.conf file
 shouldn't be edited manually.

Yeah, but in case of emergency user can change it to get server
started. Now the question is whether to mention it in documentation, I
think we can leave this decision to committer. If he thinks that it is
better to document then I will update it.

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


alter_system_v12.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-15 Thread Peter Eisentraut
On 11/14/13, 2:50 AM, Amit Kapila wrote:
 Find the rebased version attached with this mail.

Doesn't build:

openjade  -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D . -c 
/usr/share/sgml/docbook/stylesheet/dsssl/modular/catalog -d stylesheet.dsl -t 
sgml -i output-html -V html-index postgres.sgml
openjade:reference.sgml:61:3:E: cannot find alter_system.sgml; tried 
ref/alter_system.sgml, ./alter_system.sgml, ./alter_system.sgml, 
/usr/local/share/sgml/alter_system.sgml, /usr/share/sgml/alter_system.sgml
openjade:config.sgml:164:27:X: reference to non-existent ID SQL-ALTERSYSTEM
make[3]: *** [HTML.index] Error 1
make[3]: *** Deleting file `HTML.index'
osx -D. -x lower -i include-xslt-index postgres.sgml postgres.xmltmp
osx:reference.sgml:61:3:E: cannot find alter_system.sgml; tried 
ref/alter_system.sgml, ./alter_system.sgml, 
/usr/local/share/sgml/alter_system.sgml, /usr/share/sgml/alter_system.sgml
osx:config.sgml:164:27:X: reference to non-existent ID SQL-ALTERSYSTEM
make[3]: *** [postgres.xml] Error 1

New file missing in patch?


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-15 Thread Amit Kapila
On Fri, Nov 15, 2013 at 10:18 PM, Peter Eisentraut pete...@gmx.net wrote:
 On 11/14/13, 2:50 AM, Amit Kapila wrote:
 Find the rebased version attached with this mail.

 Doesn't build:

 openjade  -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D . -c 
 /usr/share/sgml/docbook/stylesheet/dsssl/modular/catalog -d stylesheet.dsl -t 
 sgml -i output-html -V html-index postgres.sgml
 openjade:reference.sgml:61:3:E: cannot find alter_system.sgml; tried 
 ref/alter_system.sgml, ./alter_system.sgml, ./alter_system.sgml, 
 /usr/local/share/sgml/alter_system.sgml, /usr/share/sgml/alter_system.sgml
 openjade:config.sgml:164:27:X: reference to non-existent ID SQL-ALTERSYSTEM
 make[3]: *** [HTML.index] Error 1
 make[3]: *** Deleting file `HTML.index'
 osx -D. -x lower -i include-xslt-index postgres.sgml postgres.xmltmp
 osx:reference.sgml:61:3:E: cannot find alter_system.sgml; tried 
 ref/alter_system.sgml, ./alter_system.sgml, 
 /usr/local/share/sgml/alter_system.sgml, /usr/share/sgml/alter_system.sgml
 osx:config.sgml:164:27:X: reference to non-existent ID SQL-ALTERSYSTEM
 make[3]: *** [postgres.xml] Error 1

 New file missing in patch?

Oops, missed the new sgml file in patch, updated patch to include it.
Many thanks for checking it.

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


alter_system_v11.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-13 Thread Haribabu kommi
On 01 October 2013 00:56 Amit Kapila wrote:
 On Mon, Sep 30, 2013 at 9:07 PM, Peter Eisentraut pete...@gmx.net
 wrote:
  On 9/28/13 3:05 AM, Amit Kapila wrote:
  Now as we have an agreement, I had updated patch for below left
 issues:
 
  Regression tests are failing.
 
Thanks for informing. I am sorry for not running regression before
 sending patch.
 
Reason for failure was that source for GUC in new function
 validate_conf_option() was hardcoded to PGC_S_FILE which was okay for
 Alter System, but
not for SET path. I had added new parameter source in this function
 and get the value of source when this is called from SET path.

Some of the initial observation of the patch are,
1. Patch is not applying against git head, needs a rebase.
2. Patch doesn't contain the tests.

I started reviewing the patch, will share the details once I finish.

Regards,
Hari babu.


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-13 Thread Amit Kapila
On Wed, Nov 13, 2013 at 4:05 PM, Haribabu kommi
haribabu.ko...@huawei.com wrote:
 On 01 October 2013 00:56 Amit Kapila wrote:
 On Mon, Sep 30, 2013 at 9:07 PM, Peter Eisentraut pete...@gmx.net
 wrote:
  On 9/28/13 3:05 AM, Amit Kapila wrote:
  Now as we have an agreement, I had updated patch for below left
 issues:
 
  Regression tests are failing.

Thanks for informing. I am sorry for not running regression before
 sending patch.

Reason for failure was that source for GUC in new function
 validate_conf_option() was hardcoded to PGC_S_FILE which was okay for
 Alter System, but
not for SET path. I had added new parameter source in this function
 and get the value of source when this is called from SET path.

 Some of the initial observation of the patch are,
 1. Patch is not applying against git head, needs a rebase.
 2. Patch doesn't contain the tests.
   It was intentional and as per feedback for this patch. As for
testing this feature, we need to put sleep after operation, so it was
suggested to remove tests.

 I started reviewing the patch, will share the details once I finish.
Thanks.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-11-13 Thread Amit Kapila
On Wed, Nov 13, 2013 at 7:23 PM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Wed, Nov 13, 2013 at 4:05 PM, Haribabu kommi
 haribabu.ko...@huawei.com wrote:
 On 01 October 2013 00:56 Amit Kapila wrote:
 On Mon, Sep 30, 2013 at 9:07 PM, Peter Eisentraut pete...@gmx.net
 wrote:
  On 9/28/13 3:05 AM, Amit Kapila wrote:
  Now as we have an agreement, I had updated patch for below left
 issues:
 

 Some of the initial observation of the patch are,
 1. Patch is not applying against git head, needs a rebase.

Find the rebased version attached with this mail. I have removed
header inclusions of port.h and lwlock.h from guc.c, it works fine
without them.

 2. Patch doesn't contain the tests.
It was intentional and as per feedback for this patch. As for
 testing this feature, we need to put sleep after operation, so it was
 suggested to remove tests.


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


alter_system_v10.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-09-30 Thread Peter Eisentraut
On 9/28/13 3:05 AM, Amit Kapila wrote:
 Now as we have an agreement, I had updated patch for below left issues:

Regression tests are failing.


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-09-30 Thread Amit Kapila
On Mon, Sep 30, 2013 at 9:07 PM, Peter Eisentraut pete...@gmx.net wrote:
 On 9/28/13 3:05 AM, Amit Kapila wrote:
 Now as we have an agreement, I had updated patch for below left issues:

 Regression tests are failing.

   Thanks for informing. I am sorry for not running regression before
sending patch.

   Reason for failure was that source for GUC in new function
validate_conf_option() was hardcoded to PGC_S_FILE which was okay for
Alter System, but
   not for SET path. I had added new parameter source in this function
and get the value of source when this is called from SET path.

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


alter_system_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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-09-28 Thread Amit Kapila
On Fri, Aug 30, 2013 at 8:24 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Stephen Frost sfr...@snowman.net writes:
 * Robert Haas (robertmh...@gmail.com) wrote:
 I think you're getting way too hung up on the fact that the proposed
 auto.conf will be stored as a flat file.  From your comments upthread,
 I gather that you'd be rejoicing if it were a table.

 I'd be happy if it was a table which managed an *independent* set of
 parameters from those used to bootstrap the system, but no one seems to
 like breaking up the options between things that can be sanely modified
 without other OS changes and things which require OS support.

 I agree with Robert's comments upthread that if the new facility can't do
 everything that can be done today by editing postgresql.conf, it's not
 going to be adequate.  So I'm not in favor of having two sets of
 parameters.  It's also not clear to me that we can make a reliable
 distinction between parameters that can prevent a server restart vs those
 that can't; or at least, the set of the latter will be much smaller than
 one could wish.

Now as we have an agreement, I had updated patch for below left issues:
1. ALTER SYSTEM SET should be constrained to only set known GUCs, this
point has been discussed on another mail thread
   (http://www.postgresql.org/message-id/14857.1378523...@sss.pgh.pa.us)
   In function AlterSystemSetConfigFile(), when we try to get the
record using find_option(), pass second parameter as false which will
make sure
   if the parameter doesn't exist it will return NULL.
2. Some indentation issues.

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


alter_system_v8.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Cédric Villemain
Le jeudi 29 août 2013 18:42:13 Stephen Frost a écrit :
 On Thursday, August 29, 2013, Andres Freund wrote:
  If you don't want your installation to use it, tell you ops people not
  to do so. They are superusers, they need to have the ability to follow
  some rules you make up internally.
 
 The OPs people are the ones that will be upset with this because the DBAs
 will be modifying configs which OPs rightfully claim as theirs. If they
 have a way to prevent it then perhaps it's not terrible but they'd also
 need to know to disable this new feature. As for ALTER DATABASE- I would
 be happier with encouraging use of that (or providing an ALTER CLUSTER) for
 those thing it makes sense and works for and removing those from being in
 postgresql.conf. I still feel things like listen_addresses shouldn't be
 changed through this.

ALTER ROLE ALL may be good enougth to handle every GUC that we can also remove 
from postgresql.conf (I suppose all GUC needing only a reload, not a restart). 
It may needs some improvement to handle changing default for ALL and adding 
new role.

  The energy wasted in a good part of this massive 550+ messages thread is
  truly saddening. We all (c|sh)ould have spent that time making PG more
  awesome instead.
 
 Perhaps not understood by all, but keeping PG awesome involves more than
 adding every feature proposed- it also means saying no sometimes; to
 features, to new GUCs, even to micro-optimizations when they're overly
 complicated and offer only minimal or questionable improvements.

Agreed, the current feature and proposal does not include pg_reload, and it 
introduces a full machinery we absolutely don't need.

Grammar can be added later when the feature is stable.

So far, we can achieve the goal by using adminpack, by using a file_fdw or a 
config_fdw. IMHO it is the job of a FDW to be able to handle atomic write or 
anything like that.

I've commented one of the proposed patch adding some helpers to validate GUC 
change, I claimed this part was good enough to be added without ALTER SYSTEM 
(so a contrib can use it).

-- 
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

signature.asc
Description: This is a digitally signed message part.


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Dimitri Fontaine
Stephen Frost sfr...@snowman.net writes:
 The OPs people are the ones that will be upset with this because the DBAs
 will be modifying configs which OPs rightfully claim as theirs.

If that's the problem you want to solve, there's no technical solution
that will put you at ease. That's a people and trust problem.

I don't think typical (or less typical) organisation should drive our
technical choices too much, and I'm pretty confident they didn't in the
past: pg_hba.conf is a file not because it's meant for this or that team
but because it makes sense technically to manage the settings to allow
using some resources *outside* of said resources.

We currently have no way that I know of to disable ALTER ROLE SET and
ALTER DATABASE SET effects, why do we need to provide that feature for
ALTER SYSTEM SET so much?

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Andres Freund
Hi,

On 2013-08-30 10:20:48 +0200, Cédric Villemain wrote:
   The energy wasted in a good part of this massive 550+ messages thread is
   truly saddening. We all (c|sh)ould have spent that time making PG more
   awesome instead.
  
  Perhaps not understood by all, but keeping PG awesome involves more than
  adding every feature proposed- it also means saying no sometimes; to
  features, to new GUCs, even to micro-optimizations when they're overly
  complicated and offer only minimal or questionable improvements.
 
 Agreed, the current feature and proposal does not include pg_reload, and it
 introduces a full machinery we absolutely don't need.

The complexity in the last version of the patch I looked at wasn't in
the added grammar or pg_reload() (well, it didn't have that). It was the
logic to read (from memory)/write the config file and validate the
GUCs. That's needed even if you put it into some module. And it requires
support from guc.c/guc-file.l

 Grammar can be added later when the feature is stable.

Could you explain the advantages of this? It will require users to get
used to different interfaces and we will end up maintaining both just
about forever. And I don't see the grammar being that contentious?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
 Stephen Frost sfr...@snowman.net writes:
  The OPs people are the ones that will be upset with this because the DBAs
  will be modifying configs which OPs rightfully claim as theirs.
 
 If that's the problem you want to solve, there's no technical solution
 that will put you at ease. That's a people and trust problem.

I really just don't buy that- I've already put forward suggestions for
how to deal with it, but no one here seems to understand the
distinction.  Modifying listen_addresses through ALTER SYSTEM is akin to
ISC/bind allowing changes to its listen_addresses equivilant through
dynamic DNS updates.  Would it be possible to implement?  Sure.  Does it
make any sense?  Certainly not.

 I don't think typical (or less typical) organisation should drive our
 technical choices too much, and I'm pretty confident they didn't in the
 past: pg_hba.conf is a file not because it's meant for this or that team
 but because it makes sense technically to manage the settings to allow
 using some resources *outside* of said resources.

I'm all for moving pg_hba.conf into the database properly, actually.  We
already handle permissions and user access in the DB and that's one of
the DBA's responsibilities.  The same goes for pg_ident.conf.

 We currently have no way that I know of to disable ALTER ROLE SET and
 ALTER DATABASE SET effects, why do we need to provide that feature for
 ALTER SYSTEM SET so much?

Because we've got crap mixed into postgresql.conf which are bootstrap
configs needed to get the system started.  Those things, in my view
anyway, fall much more into the category of resources which should be
managed outside the database than pg_hba.conf.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Cédric Villemain (ced...@2ndquadrant.com) wrote:
 ALTER ROLE ALL may be good enougth to handle every GUC that we can also 
 remove 
 from postgresql.conf (I suppose all GUC needing only a reload, not a 
 restart). 
 It may needs some improvement to handle changing default for ALL and adding 
 new role.

Yes, one of the issues with the existing system is that you can't
specify a default to be applied to new roles.  Also, there are
parameters which are not per-role yet which it probably makes sense to
be in the database and we'd need a way to deal with that.  Although, at
the same time, considering it role based does make for a nice
distinction.  Unfortunately, those clammoring for this will argue that
it wouldn't replace adminpack and that they couldn't use it to modify
their /etc/network/interfaces file, which is the obvious next step to
all of this.

 Grammar can be added later when the feature is stable.

I tend to agree w/ Andres on this point- the grammar isn't really the
contentious part.  I think I see where you were going with this in that
excluding the grammar makes it able to live as a module, but that's a
different consideration.

 I've commented one of the proposed patch adding some helpers to validate GUC 
 change, I claimed this part was good enough to be added without ALTER SYSTEM 
 (so a contrib can use it).

Perhaps..

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Andres Freund (and...@2ndquadrant.com) wrote:
 On 2013-08-30 10:20:48 +0200, Cédric Villemain wrote:
  Grammar can be added later when the feature is stable.
 
 Could you explain the advantages of this? It will require users to get
 used to different interfaces and we will end up maintaining both just
 about forever. And I don't see the grammar being that contentious?

The different interfaces issue already exists, to some extent, as
adminpack does exist already and this is clearly a different interface
from that.  That said, as I mentioned just now elsewhere, I agree that
the grammar itself (being rather simple anyway...) isn't the contentious
part of this.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Andres Freund
On 2013-08-30 08:48:21 -0400, Stephen Frost wrote:
 * Dimitri Fontaine (dimi...@2ndquadrant.fr) wrote:
  Stephen Frost sfr...@snowman.net writes:
   The OPs people are the ones that will be upset with this because the DBAs
   will be modifying configs which OPs rightfully claim as theirs.
  
  If that's the problem you want to solve, there's no technical solution
  that will put you at ease. That's a people and trust problem.
 
 I really just don't buy that- I've already put forward suggestions for
 how to deal with it, but no one here seems to understand the
 distinction.  Modifying listen_addresses through ALTER SYSTEM is akin to
 ISC/bind allowing changes to its listen_addresses equivilant through
 dynamic DNS updates.  Would it be possible to implement?  Sure.  Does it
 make any sense?  Certainly not.

I very much want to change stuff like wal_level, listen_addresses and
shared_buffers via ALTER SYSTEM. Configuration variables like that
(PGC_POSTMASTER stuff mostly) are the prime reason why you actually need
to change postgresql.conf instead of changing per user/database
settings.
And you don't even need to do anything special to implement it. Because
it's already there.

  We currently have no way that I know of to disable ALTER ROLE SET and
  ALTER DATABASE SET effects, why do we need to provide that feature for
  ALTER SYSTEM SET so much?
 
 Because we've got crap mixed into postgresql.conf which are bootstrap
 configs needed to get the system started.  Those things, in my view
 anyway, fall much more into the category of resources which should be
 managed outside the database than pg_hba.conf.

I think the problem with your position in this thread is that you want
to overhaul the way our configuration works in a pretty radical
way. Which is fair enough, there certainly are deficiencies. But it's
not the topic of this thread.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Andres Freund
On 2013-08-29 21:26:48 -0400, Stephen Frost wrote:
  Sure, you can construct a scenario where this matters.  The ops guys
  have sudo postgres pg_ctl access but adminpack isn't installed and
  they have no other way to modify the configuration file.  But that's
  just bizarre.  And if that's really the environment you have, then you
  can install a loadable module that grabs ProcessUtility_hook and uses
  it to forbid ALTER SYSTEM on that machine.  Hell, we can ship such a
  thing in contrib.  Problem solved.  But it's surely too obscure a
  combination of circumstances to justify disabling this by default.
 
 It's not the OPs guy that I'm worried about using ALTER SYSTEM- I don't
 expect them to have any clue about it or care about it, except where it
 can be used to modify things under /etc which they, rightfully, consider
 their domain.

I think for the scenarios you describe it makes far, far much more sense
to add the ability to easily monitor for two things:
* on-disk configuration isn't the same as the currently loaded (not
  trivially possible yet)
* Configuration variables only come from locations that are approved for
  in your scenario (Already possible, we might want to make it even easier)

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Robert Haas
On Thu, Aug 29, 2013 at 9:12 PM, Stephen Frost sfr...@snowman.net wrote:
 You will not find me argueing to allow that in normal operation, or most
 direct-catalog hacking.  I'd be much happier if we had all the ALTER,
 etc, options necessary to prevent any need to ever touch the catalogs.
 Similairly, it'd be nice to have more permission options which reduce
 the need for anyone to be superuser.

Sure, I agree.  But you can't burden this patch with the job of
conforming to what you and I think the project policy ought to be, but
isn't.  Right now, as things stand today, the superuser is one step
short of God Almighty.  The only way in which we even attempt to
restrict the superuser is to try to keep her from hijacking the
postgres login account.  But we don't even try to do that very
thoroughly, as has recently been pointed out on other threads; there
are several plausible attack vectors for her to do just that.  This
patch fits right into that existing philosophy.  If we take this
patch, and I think we should, and later change our policy, then the
permissions around this may require adjustment to fit the new policy.
Fine!  But that policy change is properly a separate discussion.

 Now, you can argue that people are more likely to render the database
 nonfunctional by changing GUC settings than they are to do it by
 corrupting the system catalogs, but I'm not sure I believe it.  We
 can, after all, validate that any setting a user supplies is a valid
 value for that setting before writing it out to the configuration
 file.  It might still make the system fail to start if - for example -
 it causes too many semaphores to be reserved, or something like that.
 But why should we think that such mistakes will be common?  If
 anything, it sounds less error-prone to me than hand-editing the
 configuration file, where typing something like on_exit_error=maybe
 will make the server fail to start.  We can eliminate that whole
 category of error, at least for people who choose to use the new
 tools.

 That category of error is much more likely to get caught by the sysadmin
 doing the restart after the update..

If you edit postgresql.conf manually today, you will have no warning
of ANY sort of error you might make.  With a feature like this, we can
catch a large subset of things you might do wrong *before we even
modify the file*.  Yes, there will be some that slip through, but an
imperfect mechanism for catching mistakes is better than no mechanism
at all.

 ALTER SYSTEM doesn't provide any
 way to restart the DB to make sure it worked.
 That, in turn, encourages
 modification of the config parameters w/o a restart, which is a pretty
 bad idea, imv.

vi doesn't provide any way to restart the DB to make sure it worked,
either, and never will.  However, ALTER SYSTEM could be extended in
exactly that way: we could have ALTER SYSTEM RESTART.  Of course some
people are likely to argue that's a bad idea, so we'd have to have
that discussion and think carefully about the issues, but there's
nothing intrinsic that restricts us from giving ALTER SYSTEM any
arbitrary set of capabilities we might want it to have.  Even if we
don't ever do that, we're no worse off than today.

Well, with one exception.  If we make it easier to modify the
configuration file, then people are more likely to do it, and thus
more likely to do it wrong.  But you could use that argument against
any change that improves ease-of-use.  And ease-of-use is a feature,
not a bug.

 If you're using the term dangerous to refer to a security exposure,
 I concede there is some incremental exposure from allowing this by
 default.  But it's not a terribly large additional exposure.  It's
 already the case that if adminpack is available the super-user can do
 whatever he or she wants, because she or he can write to arbitrary
 files inside the data directory.

 This is only true if -contrib is installed, which a *lot* of admins
 refuse to do, specifically because of such insane modules as this.
 Therefore, I would argue that it's not nearly as small a change wrt
 exposure as you believe.  Strictly speaking, I don't believe it's a huge
 security hole or something, but I do think it's going to surprise admins
 who aren't following the lists.

If we take the patch, it will be in the release notes, just like any
other feature.  I suspect there will be more people *pleasantly*
surprised than anything else.  If there's anything I've learned about
ease-of-use as a 10+-year veteran of PostgreSQL, it's that being able
to do everything via a database connection is really, really, really
convenient.  That's why foreign data wrappers are awesome.  Instead of
installing a foreign data wrapper for Oracle and pointing it at an
Oracle server and then sucking data down over the link to store or
process or whatever, you put all that logic in a client which knows
how to talk to both PostgreSQL and Oracle.  It turns out, though, that
having that capability *in the server* is really 

Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Andres Freund (and...@2ndquadrant.com) wrote:
 On 2013-08-30 08:48:21 -0400, Stephen Frost wrote:
  I really just don't buy that- I've already put forward suggestions for
  how to deal with it, but no one here seems to understand the
  distinction.  Modifying listen_addresses through ALTER SYSTEM is akin to
  ISC/bind allowing changes to its listen_addresses equivilant through
  dynamic DNS updates.  Would it be possible to implement?  Sure.  Does it
  make any sense?  Certainly not.
 
 I very much want to change stuff like wal_level, listen_addresses and
 shared_buffers via ALTER SYSTEM. Configuration variables like that
 (PGC_POSTMASTER stuff mostly) are the prime reason why you actually need
 to change postgresql.conf instead of changing per user/database
 settings.

wal_level and shared_buffers I can buy, but listen_addresses?  The most
typical change there is going from localhost - '*', but you've got to
be on the box to do that.  Anything else and you're going to need to be
adding interfaces to the box anyway and hacking around in
/etc/network/interfaces or what-have-you.

  Because we've got crap mixed into postgresql.conf which are bootstrap
  configs needed to get the system started.  Those things, in my view
  anyway, fall much more into the category of resources which should be
  managed outside the database than pg_hba.conf.
 
 I think the problem with your position in this thread is that you want
 to overhaul the way our configuration works in a pretty radical
 way. Which is fair enough, there certainly are deficiencies. But it's
 not the topic of this thread.

You and Robert both seem to be of the opinion that this hack which
brings postgresql.conf into the database via ALTER SYSTEM is a-ok
because it's moving us forward in someone's mind, but it really is
developing a system configuration management system which *looks* like a
first-class citizen when it actually falls woefully short of that.

There is absolutely no question in my mind that this will be a huge
support pain, from the first ALTER SYSTEM SET shared_buffers = blah;
SHOW shared_buffers; to the why can't my database start?!?  it's
complaining it can't allocate memory but I keep changing postgresql.conf
and nothing works!  I'm simply not convinced that this is moving us
forward nor that we will end up with more benefit than pain from it.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Andres Freund (and...@2ndquadrant.com) wrote:
 On 2013-08-29 21:26:48 -0400, Stephen Frost wrote:
  It's not the OPs guy that I'm worried about using ALTER SYSTEM- I don't
  expect them to have any clue about it or care about it, except where it
  can be used to modify things under /etc which they, rightfully, consider
  their domain.
 
 I think for the scenarios you describe it makes far, far much more sense
 to add the ability to easily monitor for two things:
 * on-disk configuration isn't the same as the currently loaded (not
   trivially possible yet)

That would certainly help.  I don't know that it needs to be technically
'trivial', but at least having check_postgres.pl or something which can
alert on it would be an improvement.  Clearly, that would be valuable
today (assuming it doesn't already exist somewhere..  it might).

 * Configuration variables only come from locations that are approved for
   in your scenario (Already possible, we might want to make it even easier)

That an interesting notion; do you have something specific in mind..?
The easiest, imv anyway, would be that options set in postgresql.conf
can't be overridden, but that gets us into the bootstrap problem that
people seem to be concerned about.  It would also be a change to how
postgresql.conf is parsed today which some people would be annoyed by.
Having some configuration option which says what can be modified by
alter system doesn't strike me as a terribly good solution either.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Andres Freund
On 2013-08-30 09:19:42 -0400, Stephen Frost wrote:
 * Andres Freund (and...@2ndquadrant.com) wrote:
  On 2013-08-30 08:48:21 -0400, Stephen Frost wrote:
   I really just don't buy that- I've already put forward suggestions for
   how to deal with it, but no one here seems to understand the
   distinction.  Modifying listen_addresses through ALTER SYSTEM is akin to
   ISC/bind allowing changes to its listen_addresses equivilant through
   dynamic DNS updates.  Would it be possible to implement?  Sure.  Does it
   make any sense?  Certainly not.
  
  I very much want to change stuff like wal_level, listen_addresses and
  shared_buffers via ALTER SYSTEM. Configuration variables like that
  (PGC_POSTMASTER stuff mostly) are the prime reason why you actually need
  to change postgresql.conf instead of changing per user/database
  settings.
 
 wal_level and shared_buffers I can buy, but listen_addresses?  The most
 typical change there is going from localhost - '*', but you've got to
 be on the box to do that.  Anything else and you're going to need to be
 adding interfaces to the box anyway and hacking around in
 /etc/network/interfaces or what-have-you.

Even if it requires to be on the box locally, I only need libpq for
it. And it's not infrequent to allow additional, already configured
interfaces. And even if not, what's the point of prohibiting
listen_interfaces specifically? When the other very interesting
variables have the same dangers?
Doing this on a variable-by-variable basis will a) be a ridiculous
amount of effort, b) confuse users which may not share our judgement of
individual variables.

   Because we've got crap mixed into postgresql.conf which are bootstrap
   configs needed to get the system started.  Those things, in my view
   anyway, fall much more into the category of resources which should be
   managed outside the database than pg_hba.conf.
  
  I think the problem with your position in this thread is that you want
  to overhaul the way our configuration works in a pretty radical
  way. Which is fair enough, there certainly are deficiencies. But it's
  not the topic of this thread.
 
 You and Robert both seem to be of the opinion that this hack which
 brings postgresql.conf into the database via ALTER SYSTEM is a-ok
 because it's moving us forward in someone's mind, but it really is
 developing a system configuration management system which *looks* like a
 first-class citizen when it actually falls woefully short of that.

It's what plenty of people want and it doesn't hurt people who do not
want it. Yes. I think that's a step forward.

 There is absolutely no question in my mind that this will be a huge
 support pain, from the first ALTER SYSTEM SET shared_buffers = blah;
 SHOW shared_buffers; to the why can't my database start?!?  it's
 complaining it can't allocate memory but I keep changing postgresql.conf
 and nothing works!  I'm simply not convinced that this is moving us
 forward nor that we will end up with more benefit than pain from it.

That will not show the changed shared_buffers. And it (afair) will throw
a WARNING that shared_buffers couldn't be adjusted at this point.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Robert Haas
On Thu, Aug 29, 2013 at 9:26 PM, Stephen Frost sfr...@snowman.net wrote:
 In the first place, modifying postgresql.conf and not immediately
 restarting the server to test your changes is probably the single most
 basic DBA error imaginable.

 You're not modifying postgresql.conf with ALTER SYSTEM, now are you?
 Admins are going to realize the need to restart or at least reload the
 service after updating such, but a DBA who has focused mainly on
 normalization, query optimization, etc, isn't going to have the same
 experience that a sysadmin has.

I refuse to reject any feature on the basis of the possibility that
people might use it without reading the documentation.  Nearly
anything anyone will ever propose is so idiot-proof that we can't
conceive of a scenario in which someone shoots their foot off with it.
 The question is whether the usefulness of the feature exceeds, by a
sufficient amount, the harm it's likely to do, and I think the answer
is clearly yes.

 A DBA updating a GUC, something they are likely to do frequently
 day-in and day-out, isn't necessairly going to consider that it's a
 reboot-needing change.  Indeed, it's not unreasonable to consider that
 we may, some day, actually be able to change or increase
 shared_buffers on the fly (or error in the trying); I'd think your
 work with the dynamic backends would actually make that likely to
 happen sooner rather than later.

I wouldn't hold my breath.  We have way too many separate,
variable-length data structures in shared memory.  Increasing
shared_buffers means making the lwlock array bigger, making the buffer
header array bigger, allocating more space for the buffer mapping
hash, etc.  I doubt we'd accept a design that involves each of those
data structures being a separate shared memory segment, but if they're
all in the same segment one after another, then you can't easily
extend them on the fly.

There are also a lot of problems around unmapping-and-remapping a
segment.  If the remap fails, it's going to be at least a FATAL, but
if you had any shared memory state in the unmapped segment (like a
buffer pin) that has to be unwound, you have to PANIC; there's no
other way to fix it.  My initial design for dynamic shared memory
allowed for an unbounded number of dynamic shared memory segments by
growing the control segment on the fly.  However, this introduced
PANIC hazards in corner cases that I couldn't get rid of, so now
there's a fairly generous but fixed-at-startup-time limit on how many
segments you can have.  In practice I don't think this matters much,
but it was a sobering reminder that the main shared memory segment,
with all of its inflexibility, has important reliability properties
that are hard to replicate in more dynamic scenarios.

 It's not the OPs guy that I'm worried about using ALTER SYSTEM- I don't
 expect them to have any clue about it or care about it, except where it
 can be used to modify things under /etc which they, rightfully, consider
 their domain.

Under the currently-proposed design, it can't be used to do any such
thing.  It can only be used to modify some auto.conf file which lives
in $PGDATA.  It's therefore no different from the ops perspective than
ALTER DATABASE or ALTER USER - except that it allows all settings to
be changed rather than only a subset.

-- 
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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Robert Haas
On Fri, Aug 30, 2013 at 8:53 AM, Stephen Frost sfr...@snowman.net wrote:
 Yes, one of the issues with the existing system is that you can't
 specify a default to be applied to new roles.  Also, there are
 parameters which are not per-role yet which it probably makes sense to
 be in the database and we'd need a way to deal with that.  Although, at
 the same time, considering it role based does make for a nice
 distinction.  Unfortunately, those clammoring for this will argue that
 it wouldn't replace adminpack and that they couldn't use it to modify
 their /etc/network/interfaces file, which is the obvious next step to
 all of this.

This is a straw-man.  adminpack doesn't allow reading or writing files
outside of the configured data and log directories, as a security
precaution.  But suppose it did.  If your permissions on
/etc/network/interfaces allow the postgres user to modify it, then you
pretty much deserve exactly what you get.  Likening this to the
feature being proposed is silly.  What is being asked for here is the
ability to easily modify system-wide settings from the SQL prompt,
just as today you can modify settings per-user or per-database.
That's not the same thing as rewriting the entire system
configuration.

-- 
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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Andres Freund (and...@2ndquadrant.com) wrote:
 On 2013-08-30 09:19:42 -0400, Stephen Frost wrote:
  wal_level and shared_buffers I can buy, but listen_addresses?  The most
  typical change there is going from localhost - '*', but you've got to
  be on the box to do that.  Anything else and you're going to need to be
  adding interfaces to the box anyway and hacking around in
  /etc/network/interfaces or what-have-you.
 
 Even if it requires to be on the box locally, I only need libpq for
 it. And it's not infrequent to allow additional, already configured
 interfaces. And even if not, what's the point of prohibiting
 listen_interfaces specifically? When the other very interesting
 variables have the same dangers?

I'd like to see those dangers removed from the other very interesting
variables.  We're making progress towards that, for example with
shared_buffers.  I've commented on that already up-thread.  Hell, you
could even make things such that PG would start w/ a misconfigured
listen_addresses- but if we don't like that then I would argue that it
shouldn't be included here.  I'm not entirely sure how wal_level has the
same danger as the others..

 Doing this on a variable-by-variable basis will a) be a ridiculous
 amount of effort, b) confuse users which may not share our judgement of
 individual variables.

I don't think the effort involved is nearly as much as you claim and we
already have the issue that users don't like our choices around what can
be reconfigured on the fly and what can't (perhaps they understand that
there are technical challenges to some of them, but that doesn't make
them agree with them).

  You and Robert both seem to be of the opinion that this hack which
  brings postgresql.conf into the database via ALTER SYSTEM is a-ok
  because it's moving us forward in someone's mind, but it really is
  developing a system configuration management system which *looks* like a
  first-class citizen when it actually falls woefully short of that.
 
 It's what plenty of people want and it doesn't hurt people who do not
 want it. Yes. I think that's a step forward.

It will be quite interesting to see if people decide they really wanted
this once they actually *get* it.

  There is absolutely no question in my mind that this will be a huge
  support pain, from the first ALTER SYSTEM SET shared_buffers = blah;
  SHOW shared_buffers; to the why can't my database start?!?  it's
  complaining it can't allocate memory but I keep changing postgresql.conf
  and nothing works!  I'm simply not convinced that this is moving us
  forward nor that we will end up with more benefit than pain from it.
 
 That will not show the changed shared_buffers. And it (afair) will throw
 a WARNING that shared_buffers couldn't be adjusted at this point.

Not showing the change is what I was getting at.  As has been said
elsewhere, throwing a warning on every interesting invokation of a
command can speak to it not being exactly 'ready'.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Andres Freund
On 2013-08-30 09:43:01 -0400, Stephen Frost wrote:
 * Andres Freund (and...@2ndquadrant.com) wrote:
  On 2013-08-30 09:19:42 -0400, Stephen Frost wrote:
   wal_level and shared_buffers I can buy, but listen_addresses?  The most
   typical change there is going from localhost - '*', but you've got to
   be on the box to do that.  Anything else and you're going to need to be
   adding interfaces to the box anyway and hacking around in
   /etc/network/interfaces or what-have-you.
  
  Even if it requires to be on the box locally, I only need libpq for
  it. And it's not infrequent to allow additional, already configured
  interfaces. And even if not, what's the point of prohibiting
  listen_interfaces specifically? When the other very interesting
  variables have the same dangers?
 
 I'd like to see those dangers removed from the other very interesting
 variables.  We're making progress towards that, for example with
 shared_buffers.  I've commented on that already up-thread.  Hell, you
 could even make things such that PG would start w/ a misconfigured
 listen_addresses- but if we don't like that then I would argue that it
 shouldn't be included here.

Yes, we're making progress. But that's an independent thing, partially
constrained by implementation complexity, partially constrained by cross
platform worries.
You're free to work on reducing the dangers around other variables, but
I don't understand in the very least why that should stop this feature.

 I'm not entirely sure how wal_level has the same danger as the
 others..

Try setting max_wal_senders = 10 and wal_level = minimal.

   There is absolutely no question in my mind that this will be a huge
   support pain, from the first ALTER SYSTEM SET shared_buffers = blah;
   SHOW shared_buffers; to the why can't my database start?!?  it's
   complaining it can't allocate memory but I keep changing postgresql.conf
   and nothing works!  I'm simply not convinced that this is moving us
   forward nor that we will end up with more benefit than pain from it.
  
  That will not show the changed shared_buffers. And it (afair) will throw
  a WARNING that shared_buffers couldn't be adjusted at this point.
 
 Not showing the change is what I was getting at.  As has been said
 elsewhere, throwing a warning on every interesting invokation of a
 command can speak to it not being exactly 'ready'.

Then pg isn't ready already, because it has done that for a long
time. Set some PGC_POSTMASTER variable and reload the configuration.

Guiding a user that to perform required further action doesn't imply
nonreadyness in the least.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 there's a fairly generous but fixed-at-startup-time limit on how many
 segments you can have.  In practice I don't think this matters much,
 but it was a sobering reminder that the main shared memory segment,
 with all of its inflexibility, has important reliability properties
 that are hard to replicate in more dynamic scenarios.

Why wouldn't it be possible to have the same arrangment for
shared_buffers, where you have more entires than you 'need' at startup
but which then allows you to add more shared segments later?  I do see
that we would need an additional bit of indirection to handle that,
which might be too much overhead, but the concept seems possible.  Isn't
that more-or-less how the kernel handles dynamic memory..?

 Under the currently-proposed design, it can't be used to do any such
 thing.  It can only be used to modify some auto.conf file which lives
 in $PGDATA.  It's therefore no different from the ops perspective than
 ALTER DATABASE or ALTER USER - except that it allows all settings to
 be changed rather than only a subset.

Claiming that modifying a file *included from a file in /etc* doesn't
modify things under /etc is disingenuous, imv.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Robert Haas
On Fri, Aug 30, 2013 at 9:19 AM, Stephen Frost sfr...@snowman.net wrote:
 You and Robert both seem to be of the opinion that this hack which
 brings postgresql.conf into the database via ALTER SYSTEM is a-ok
 because it's moving us forward in someone's mind, but it really is
 developing a system configuration management system which *looks* like a
 first-class citizen when it actually falls woefully short of that.

It's not a system configuration management system, and it doesn't
pretend to be.  It's an analogue of ALTER USER and ALTER DATABASE that
papers over their shortcomings, and a safer alternative to adminpack's
kludgy way of enabling the same functionality.

 There is absolutely no question in my mind that this will be a huge
 support pain, from the first ALTER SYSTEM SET shared_buffers = blah;
 SHOW shared_buffers;

They'd have the same problem with ALTER USER SET work_mem = blah.
You're acting like ALTER commands that don't take immediate effect are
something brand-new, but we've had them for years.

  to the why can't my database start?!?  it's
 complaining it can't allocate memory but I keep changing postgresql.conf
 and nothing works!

As of 9.3, that failure mode doesn't really happen any more, unless
you maybe set shared_buffers equal to 100% of system memory.

 I'm simply not convinced that this is moving us
 forward nor that we will end up with more benefit than pain from it.

Fair enough, but I'm not convinced that we'll derive any pain at all
from it.  The existing similar features haven't been a notable source
of complaints AFAIK.

-- 
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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Andres Freund (and...@2ndquadrant.com) wrote:
 Technically trivial in the sense that it should be queryable from SQL
 without having to write code in an untrusted PL ;).

hah.

 I guess storing the file modification date along the file/location a GUC
 is originating from would be good enough. Then you could write a query
 using pg_stat_file() to make sure they are up2date.

Wouldn't you want to actually look at the GUC and see if the value is
different also?  Just knowing that postgresql.conf changed doesn't mean
you want every value to say it's different...  It's entirely possible
that the file was rewritten or touch'd but the configuration is
identical.

   * Configuration variables only come from locations that are approved for
 in your scenario (Already possible, we might want to make it even 
   easier)
  
  That an interesting notion; do you have something specific in mind..?
  The easiest, imv anyway, would be that options set in postgresql.conf
  can't be overridden, but that gets us into the bootstrap problem that
  people seem to be concerned about.  It would also be a change to how
  postgresql.conf is parsed today which some people would be annoyed by.
  Having some configuration option which says what can be modified by
  alter system doesn't strike me as a terribly good solution either.
 
 I think changing the precedence of options in postgresql.conf has about
 zero chance.

Sadly, you're probably right.

 That would make it possible to easily write a query that works across
 intallation that warns about any values stored in auto.conf, even if
 they are overwritten by a per-user config or similar.

I had the impression that 'approved for' above meant something which
actually *enforced* it rather than just another monitoring check..

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Robert Haas
On Fri, Aug 30, 2013 at 9:49 AM, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@gmail.com) wrote:
 there's a fairly generous but fixed-at-startup-time limit on how many
 segments you can have.  In practice I don't think this matters much,
 but it was a sobering reminder that the main shared memory segment,
 with all of its inflexibility, has important reliability properties
 that are hard to replicate in more dynamic scenarios.

 Why wouldn't it be possible to have the same arrangment for
 shared_buffers, where you have more entires than you 'need' at startup
 but which then allows you to add more shared segments later?  I do see
 that we would need an additional bit of indirection to handle that,
 which might be too much overhead, but the concept seems possible.  Isn't
 that more-or-less how the kernel handles dynamic memory..?

Yeah, I think that something like would be possible, but the
synchronization would be tricky, and it wouldn't work at all with
System V shared memory or Windows shared memory, which apparently
can't be resized after creation.  Also, it would rely on the system
administrator having a sensible setting for
max_on_the_fly_shared_buffers and only having the wrong setting for
initial_shared_buffers_until_i_change_it_later, which might not cover
a very high percentage of the cases that arise in practice.

 Under the currently-proposed design, it can't be used to do any such
 thing.  It can only be used to modify some auto.conf file which lives
 in $PGDATA.  It's therefore no different from the ops perspective than
 ALTER DATABASE or ALTER USER - except that it allows all settings to
 be changed rather than only a subset.

 Claiming that modifying a file *included from a file in /etc* doesn't
 modify things under /etc is disingenuous, imv.

I think you're getting way too hung up on the fact that the proposed
auto.conf will be stored as a flat file.  From your comments upthread,
I gather that you'd be rejoicing if it were a table.  The only reason
we're not doing that is because of the possibility that something in
auto.conf might keep the server from starting.  I don't think that's
gonna be very common now that we're mostly out from under the System V
shared memory limits, but we'll see.  As you point out, it might also
be worth fixing: maybe we can find some way to arrange things so that
the server will always be able to start up regardless of how badly
messed-up auto.conf is... but that's a different patch.

-- 
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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Fri, Aug 30, 2013 at 8:53 AM, Stephen Frost sfr...@snowman.net wrote:
  Yes, one of the issues with the existing system is that you can't
  specify a default to be applied to new roles.  Also, there are
  parameters which are not per-role yet which it probably makes sense to
  be in the database and we'd need a way to deal with that.  Although, at
  the same time, considering it role based does make for a nice
  distinction.  Unfortunately, those clammoring for this will argue that
  it wouldn't replace adminpack and that they couldn't use it to modify
  their /etc/network/interfaces file, which is the obvious next step to
  all of this.
 
 This is a straw-man.  

It was intended to be and I wasn't particularly expecting a response to
the sarcasm, at the same time..

 Likening this to the
 feature being proposed is silly.  What is being asked for here is the
 ability to easily modify system-wide settings from the SQL prompt,
 just as today you can modify settings per-user or per-database.

/etc/network/interfaces would be considered part of the system by
some.. :)  PG isn't an OS, yet..

 That's not the same thing as rewriting the entire system
 configuration.

And here you're using the *other* meaning of system, no?  Glad there
won't be any confusion here! ;)  (yes, I'm kidding..).

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
Tom, all,

  I'm not one to give up a fight (I hope that's something ya'll like
  about me ;), but in this case I'm gonna have to concede.  Clearly, I'm
  in the minority about this, at least on the lists and among the active
  hackers.  Let me just say that I hope all the happy users of this will
  outweigh the complaints.  I suppose there's only one way to find out.

Thanks!

Stephen

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  * Robert Haas (robertmh...@gmail.com) wrote:
  I think you're getting way too hung up on the fact that the proposed
  auto.conf will be stored as a flat file.  From your comments upthread,
  I gather that you'd be rejoicing if it were a table.  
 
  I'd be happy if it was a table which managed an *independent* set of
  parameters from those used to bootstrap the system, but no one seems to
  like breaking up the options between things that can be sanely modified
  without other OS changes and things which require OS support.
 
 I agree with Robert's comments upthread that if the new facility can't do
 everything that can be done today by editing postgresql.conf, it's not
 going to be adequate.  So I'm not in favor of having two sets of
 parameters.  It's also not clear to me that we can make a reliable
 distinction between parameters that can prevent a server restart vs those
 that can't; or at least, the set of the latter will be much smaller than
 one could wish.
 
   regards, tom lane


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 I think you're getting way too hung up on the fact that the proposed
 auto.conf will be stored as a flat file.  From your comments upthread,
 I gather that you'd be rejoicing if it were a table.  

I'd be happy if it was a table which managed an *independent* set of
parameters from those used to bootstrap the system, but no one seems to
like breaking up the options between things that can be sanely modified
without other OS changes and things which require OS support.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-30 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes:
 * Robert Haas (robertmh...@gmail.com) wrote:
 I think you're getting way too hung up on the fact that the proposed
 auto.conf will be stored as a flat file.  From your comments upthread,
 I gather that you'd be rejoicing if it were a table.  

 I'd be happy if it was a table which managed an *independent* set of
 parameters from those used to bootstrap the system, but no one seems to
 like breaking up the options between things that can be sanely modified
 without other OS changes and things which require OS support.

I agree with Robert's comments upthread that if the new facility can't do
everything that can be done today by editing postgresql.conf, it's not
going to be adequate.  So I'm not in favor of having two sets of
parameters.  It's also not clear to me that we can make a reliable
distinction between parameters that can prevent a server restart vs those
that can't; or at least, the set of the latter will be much smaller than
one could wish.

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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Robert Haas
On Wed, Aug 28, 2013 at 3:10 PM, Stephen Frost sfr...@snowman.net wrote:
 * Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  While I appreciate that there are bootstrap-type issues with this, I
  really don't like this idea of later stuff can just override earlier
  stuff.

  include files and conf.d-style options are for breaking the config up,
  not to allow you to override options because a file came later than an
  earlier file.  Our particular implementation of config-file reading
  happens to lend itself to later-definition-wins, but that's really
  counter-intuitive for anyone unfamiliar with PG, imv.

 I don't follow this argument at all.  Do you know any software with text
 config files that will act differently from this if the same setting is
 listed twice?  Last one wins is certainly what I'd expect.

 Have you tried having the same mount specified in multiple files in
 fstab.d..?  Also, aiui (not a big exim fan personally), duplicate
 definitions in an exim4/conf.d would result in an error.  Playing around
 in apache2/sites-enabled, it appears to be first wins wrt virtual
 hosts.

 There's a number of cases where only a single value is being set and
 subseqeunt files are 'additive' (eg: ld.so.conf.d), so they don't have
 this issue.  Similar to that are script directories, which simply run a
 set of scripts in the $DIR.d and, as it's the scripts themselves which
 are the 'parameters', and being files in a directory, you can't
 duplicate them.  Others (eg: pam.d) define the file name to be an
 enclosing context, again preventing duplication by using the filename
 itself.

 There are counter-examples also; sysctl.d will replace what's already
 been set, so perhaps it simply depends on an individual's experience.
 For my part, I'd much prefer an error or warning saying you've got
 duplicate definitions of X than a last-wins approach, though perhaps
 that's just because I like things to be very explicit and clear.
 Allowing multiple definitions of the same parameter to be valid isn't
 'clear' to me.

This may be true, but I think it's got little if anything to do with
the current patch.  There are lots of things that I don't like about
our GUC machinery, too, but the goal of this thread shouldn't be to
fix them all, but to get a useful piece of functionality added.

The thing that we should keep in mind here is that it's *already*
possible - TODAY - for users to overwrite postgresql.conf with a new
set of settings.  pgAdmin has this functionality built-in; it only
requires that you install the adminpack contrib module, which we
ship.  According to Dave Page, pgAdmin will even offer to run CREATE
EXTENSION adminpack for you, if you haven't done that already.  Also
according to Dave Page, if two users try to use it concurrently, you
will get exactly the sort of awful results that you might expect.  If
it gets killed halfway through rewriting the file, too bad for you.

The sorts of watered-down half-features being proposed here are not
going to do anything to address that situation.  If there are
restrictions on what GUCs can be changed with this feature, or if the
feature is disabled by default, or if you can only use it when the
moon is full and you hop on your left foot while spinning around
sideways, then pgAdmin (and any other, similar tools) are just going
to keep doing what they do today in the same crappy, unsafe way they
currently do it.  Meanwhile, people who use psql are going to continue
to find themselves without a reasonable API for doing the same thing
that can be done easily using pgAdmin.

I think the goals of this patch should be to (1) let users of other
clients manipulate the startup configuration just as easily as we can
already do using pgAdmin and (2) remove some of the concurrency
hazards that pgAdmin introduces, for example by using locking and
atomic renames.  Restricting the functionality to something less than
what pgAdmin provides will just cause people to ignore the new
mechanism and use the one that already exists and, by and large,
works.  And trying to revise other aspects of how GUCs and config
files work as part of this effort is not reasonably related to this
patch, and should be kept out of the discussion.

-- 
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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Stephen Frost
Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
 The sorts of watered-down half-features being proposed here are not
 going to do anything to address that situation.  If there are
 restrictions on what GUCs can be changed with this feature, or if the
 feature is disabled by default, or if you can only use it when the
 moon is full and you hop on your left foot while spinning around
 sideways, then pgAdmin (and any other, similar tools) are just going
 to keep doing what they do today in the same crappy, unsafe way they
 currently do it.  Meanwhile, people who use psql are going to continue
 to find themselves without a reasonable API for doing the same thing
 that can be done easily using pgAdmin.

To be honest, I don't find the arguments of pgAdmin does it badly
nor psql users want this ability (which I find difficult to believe)
to be particularlly compelling reasons to have a dangerous
implementation (even if it's better than 'adminpack' today) in core.

If it's in core rather than in contrib it's going to be deployed a great
deal farther with a large increase in userbase.  I've already stated
that if this is in contrib that my concerns are much less.

 I think the goals of this patch should be to (1) let users of other
 clients manipulate the startup configuration just as easily as we can
 already do using pgAdmin,

Which could be done already through use of adminpack..  The capabilities
exposed there could be used by other clients.  The fact that none of the
other clients have chosen to do so could be an indication that this
ability isn't terribly 'in demand'.

 and (2) remove some of the concurrency
 hazards that pgAdmin introduces, for example by using locking and
 atomic renames.

Why can't adminpack do that..?

 Restricting the functionality to something less than
 what pgAdmin provides will just cause people to ignore the new
 mechanism and use the one that already exists and, by and large,
 works.  And trying to revise other aspects of how GUCs and config
 files work as part of this effort is not reasonably related to this
 patch, and should be kept out of the discussion.

We're talking about modifying config files through an interface and you
wish to exclude all discussion about how those config files are handled.
That leads to a result that only an adminpack-like solution is an
option, to which I respond use and improve adminpack then.  If we want
something that works well in *core* then I don't think we can exclude
how core reads and handles config files from the discussion.  We need a
real solution, not another adminpack-like hack.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Robert Haas
On Thu, Aug 29, 2013 at 1:42 PM, Stephen Frost sfr...@snowman.net wrote:
 To be honest, I don't find the arguments of pgAdmin does it badly
 nor psql users want this ability (which I find difficult to believe)
 to be particularlly compelling reasons to have a dangerous
 implementation (even if it's better than 'adminpack' today) in core.

I don't understand how you can find that difficult to believe.  I'm a
psql user and I want it.  Josh Berkus is a psql user and he wants it.
And there are numerous statements of support on these threads from
other people as well.  The sheer volume of discussion on this topic,
and the fact that it has not gone away after years of wrangling, is a
clear indication that people do, in fact, want it.

To be honest, I think the argument that this is dangerous is pretty
ridiculous.  AFAICS, the only argument anyone's advanced for this
being dangerous is the theory that you might accidentally put
something in your postgresql.conf file that makes the server not
start.  However, the reality is that the superuser has MANY, MANY ways
of killing the database cluster as things stand.  Consider the
ever-popular DELETE FROM pg_proc.  That will not only render your
database unusable, but it's a hell of a lot harder to recover from
than anything you might do to postgresql.conf.  Therefore, from where
I'm sitting, this is like telling a head of state with the ability to
launch nuclear weapons which will destroy the planet that he isn't
allowed to bring his swiss army knife on board an aircraft.

Now, you can argue that people are more likely to render the database
nonfunctional by changing GUC settings than they are to do it by
corrupting the system catalogs, but I'm not sure I believe it.  We
can, after all, validate that any setting a user supplies is a valid
value for that setting before writing it out to the configuration
file.  It might still make the system fail to start if - for example -
it causes too many semaphores to be reserved, or something like that.
But why should we think that such mistakes will be common?  If
anything, it sounds less error-prone to me than hand-editing the
configuration file, where typing something like on_exit_error=maybe
will make the server fail to start.  We can eliminate that whole
category of error, at least for people who choose to use the new
tools.

If you're using the term dangerous to refer to a security exposure,
I concede there is some incremental exposure from allowing this by
default.  But it's not a terribly large additional exposure.  It's
already the case that if adminpack is available the super-user can do
whatever he or she wants, because she or he can write to arbitrary
files inside the data directory.  Even if not, for most intents and
purposes, ALTER DATABASE my_main_database SET whatever = thing is
functionally equivalent to modifying postgresql.conf.  Some settings
can't be modified that way, but so what?  AFAICS, about the worst
thing the user can do is ALTER SYSTEM SET shared_preload_libraries =
'rootkit'.  But if the user has the ability to install rootkit.so, the
sysadmin is already screwed.  And aside from that sort of thing, I
don't really see what can happen that's any worse than what a rogue
superuser can already do as things stand today.

 If it's in core rather than in contrib it's going to be deployed a great
 deal farther with a large increase in userbase.  I've already stated
 that if this is in contrib that my concerns are much less.

I don't really see a compelling reason why it can't or shouldn't be in
core.  But having something better in contrib would still be an
improvement on the status quo.

 I think the goals of this patch should be to (1) let users of other
 clients manipulate the startup configuration just as easily as we can
 already do using pgAdmin,

 Which could be done already through use of adminpack..  The capabilities
 exposed there could be used by other clients.  The fact that none of the
 other clients have chosen to do so could be an indication that this
 ability isn't terribly 'in demand'.

Huh?  The problem with adminpack is that it doesn't let you modify
individual configuration settings.  All you can do is rewrite an
entire file.  I guess somebody could write a specialized client that
just uses that infrastructure to rewrite postgresql.conf.  For all I
know, someone has.  Even if not, I don't think that you can use that
to prove that people don't care about this feature.  If nobody cares,
why are there 400 emails on this topic?!

 and (2) remove some of the concurrency
 hazards that pgAdmin introduces, for example by using locking and
 atomic renames.

 Why can't adminpack do that..?

It could.  But it doesn't.  We could improve it to do that, and that
might be worthwhile, but it still wouldn't be as nice as what's being
proposed here.

 Restricting the functionality to something less than
 what pgAdmin provides will just cause people to ignore the new
 mechanism and use the one that already 

Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Andres Freund
On 2013-08-29 15:07:35 -0400, Robert Haas wrote:
 On Thu, Aug 29, 2013 at 1:42 PM, Stephen Frost sfr...@snowman.net wrote:
  To be honest, I don't find the arguments of pgAdmin does it badly
  nor psql users want this ability (which I find difficult to believe)
  to be particularlly compelling reasons to have a dangerous
  implementation (even if it's better than 'adminpack' today) in core.
 
 I don't understand how you can find that difficult to believe.  I'm a
 psql user and I want it.  Josh Berkus is a psql user and he wants it.
 And there are numerous statements of support on these threads from
 other people as well.  The sheer volume of discussion on this topic,
 and the fact that it has not gone away after years of wrangling, is a
 clear indication that people do, in fact, want it.
 
 To be honest, I think the argument that this is dangerous is pretty
 ridiculous.

+waytoomuch.

  If it's in core rather than in contrib it's going to be deployed a great
  deal farther with a large increase in userbase.  I've already stated
  that if this is in contrib that my concerns are much less.
 
 I don't really see a compelling reason why it can't or shouldn't be in
 core.  But having something better in contrib would still be an
 improvement on the status quo.

I don't see much argument for putting it into contrib. One class of
users this will benefit is relatively new ones, possibly using some
GUI. Adding some additional complexity for them to enable the feature
seems pointless to me.

If you don't want your installation to use it, tell you ops people not
to do so. They are superusers, they need to have the ability to follow
some rules you make up internally.

The energy wasted in a good part of this massive 550+ messages thread is
truly saddening. We all (c|sh)ould have spent that time making PG more
awesome instead.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Stephen Frost
On Thursday, August 29, 2013, Andres Freund wrote:

 On 2013-08-29 15:07:35 -0400, Robert Haas wrote:
  I don't really see a compelling reason why it can't or shouldn't be in
  core.  But having something better in contrib would still be an
  improvement on the status quo.

 I don't see much argument for putting it into contrib. One class of
 users this will benefit is relatively new ones, possibly using some
 GUI. Adding some additional complexity for them to enable the feature
 seems pointless to me.


I keep wondering what this fantastic new GUI that isn't pgAdmin is, and why
it wouldn't be able to use the exact same mechanism that pgAdmin uses and
provides in a few simple steps to enable this.


 If you don't want your installation to use it, tell you ops people not
 to do so. They are superusers, they need to have the ability to follow
 some rules you make up internally.


The OPs people are the ones that will be upset with this because the DBAs
will be modifying configs which OPs rightfully claim as theirs. If they
have a way to prevent it then perhaps it's not terrible but they'd also
need to know to disable this new feature. As for ALTER DATABASE- I would
be happier with encouraging use of that (or providing an ALTER CLUSTER) for
those thing it makes sense and works for and removing those from being in
postgresql.conf. I still feel things like listen_addresses shouldn't be
changed through this.


 The energy wasted in a good part of this massive 550+ messages thread is
 truly saddening. We all (c|sh)ould have spent that time making PG more
 awesome instead.


Perhaps not understood by all, but keeping PG awesome involves more than
adding every feature proposed- it also means saying no sometimes; to
features, to new GUCs, even to micro-optimizations when they're overly
complicated and offer only minimal or questionable improvements.

Thanks,

Stephen


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Andres Freund
On 2013-08-29 18:42:13 -0400, Stephen Frost wrote:
 On Thursday, August 29, 2013, Andres Freund wrote:
  The energy wasted in a good part of this massive 550+ messages thread is
  truly saddening. We all (c|sh)ould have spent that time making PG more
  awesome instead.

 Perhaps not understood by all, but keeping PG awesome involves more than
 adding every feature proposed- it also means saying no sometimes; to
 features, to new GUCs, even to micro-optimizations when they're overly
 complicated and offer only minimal or questionable improvements.

But that doesn't mean that endlessly discussing in circles is a
worthwile thing to do. The discussion essentially hasn't progressed
towards concensus in the last months at all besides involving most
active hackers at some point. If anything it has become more
contentious.

Obviously lots of people have strong opinions. Now we need to make
decisions. Even if it ends up being ones I judge to be ridiculously bad.

  On 2013-08-29 15:07:35 -0400, Robert Haas wrote:
   I don't really see a compelling reason why it can't or shouldn't be in
   core.  But having something better in contrib would still be an
   improvement on the status quo.
 
  I don't see much argument for putting it into contrib. One class of
  users this will benefit is relatively new ones, possibly using some
  GUI. Adding some additional complexity for them to enable the feature
  seems pointless to me.

 I keep wondering what this fantastic new GUI that isn't pgAdmin is, and why
 it wouldn't be able to use the exact same mechanism that pgAdmin uses and
 provides in a few simple steps to enable this.

To quote Robert two mails up:

 Huh?  The problem with adminpack is that it doesn't let you modify
 individual configuration settings.  All you can do is rewrite an
 entire file.  I guess somebody could write a specialized client that
 just uses that infrastructure to rewrite postgresql.conf.  For all I
 know, someone has.  Even if not, I don't think that you can use that
 to prove that people don't care about this feature.  If nobody cares,
 why are there 400 emails on this topic?!

Also, doing it the adminpack way lacks even the most basic validity
checks. And that's not really changeable.

Presumably one major reason why we don't have other|good GUIs is that
it's ridicuously hard to make them work to an interesting extent with
the current infrastructure.

  If you don't want your installation to use it, tell you ops people not
  to do so. They are superusers, they need to have the ability to follow
  some rules you make up internally.

 The OPs people are the ones that will be upset with this because the DBAs
 will be modifying configs which OPs rightfully claim as theirs. If they
 have a way to prevent it then perhaps it's not terrible but they'd also
 need to know to disable this new feature. As for ALTER DATABASE- I would
 be happier with encouraging use of that (or providing an ALTER CLUSTER) for
 those thing it makes sense and works for and removing those from being in
 postgresql.conf. I still feel things like listen_addresses shouldn't be
 changed through this.

If they give out superuser access it has to be to people who can follow
rules. After all they don't DROP DATABASE; DELETE FROM pg_class; alter
passwords; use adminpack (changing postgresql.conf..); ... All of which
they can do.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Stephen Frost
On Thursday, August 29, 2013, Andres Freund wrote:

 To quote Robert two mails up:

  Huh?  The problem with adminpack is that it doesn't let you modify
  individual configuration settings.  All you can do is rewrite an
  entire file.


That's clearly fixable.

 I guess somebody could write a specialized client that
  just uses that infrastructure to rewrite postgresql.conf.  For all I
  know, someone has.  Even if not, I don't think that you can use that
  to prove that people don't care about this feature.  If nobody cares,
  why are there 400 emails on this topic?!


Having 400 emails about it means it's contentious. That's quite different
from having a large demand. It does speak to the author's persistence as
well, but that shouldn't be a surprise.


 Also, doing it the adminpack way lacks even the most basic validity
 checks. And that's not really changeable.


I don't see why..?  Admin pack could certainly be modified to take a
parameter and do appropriate verification before locking an object and
rewriting the file. It's what we're being expected to do in core, after
all. Indeed, we can't even do validity checks on all the options, which is
the crux of what I'm concerned about.


 Presumably one major reason why we don't have other|good GUIs is that
 it's ridicuously hard to make them work to an interesting extent with
 the current infrastructure.


Yet no one has tried to improve admin pack?


 If they give out superuser access it has to be to people who can follow
 rules. After all they don't DROP DATABASE; DELETE FROM pg_class; alter
 passwords; use adminpack (changing postgresql.conf..); ... All of which
 they can do.


This completely misses, or perhaps just ignores, the point. Disallowing
super user access can be difficult because there's a lot of *normal* DBA
activities which can't be easily done without it (like changing table
ownership or similar).  The createrole option definitely improved things
but we aren't there yet. It's certainly easy to simply not install the
adminpack. The other concerns above are strawmen because they attack a
malicious DBA.  I'm not talking about malicious DBAs but rather a generally
knowledgable DBA who changed shared_buffers up too high and then leaves on
vacation, while the OPs guys need to do a database restart for whatever
reason and then discover it doesn't start.

I bring up these concerns because I have environments where I can see
exactly this happening and I have a hard time believing that I'm somehow
alone.

Thanks,

Stephen


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Robert Haas
On Thu, Aug 29, 2013 at 8:07 PM, Stephen Frost sfr...@snowman.net wrote:
 Having 400 emails about it means it's contentious. That's quite different
 from having a large demand. It does speak to the author's persistence as
 well, but that shouldn't be a surprise.

Yet you can't ignore the fact that many people on these threads want
some form of this.

 Presumably one major reason why we don't have other|good GUIs is that
 it's ridicuously hard to make them work to an interesting extent with
 the current infrastructure.

 Yet no one has tried to improve admin pack?

No, but multiple people have tried to do ALTER SYSTEM .. SET.  Peter
had a crack at this problem, I fooled around with it (though I don't
think I ever got as far as publishing), and there were various others
as well (Greg Smith?).

 I'm not talking about malicious DBAs but rather a generally
 knowledgable DBA who changed shared_buffers up too high and then leaves on
 vacation, while the OPs guys need to do a database restart for whatever
 reason and then discover it doesn't start.

/me looks at Stephen incredulously.

In the first place, modifying postgresql.conf and not immediately
restarting the server to test your changes is probably the single most
basic DBA error imaginable.  I have a hard time viewing such a person
as generally knowledgeable.  I hope the DBA in question doesn't have
access to the switches, because he's probably the sort of guy who
reassigns switch ports and doesn't type wr m afterwards.

In the second place, the same guy can do the same thing today.  He
just has to use vi.  In fact, I think this is a pretty common
failure mode in poorly-controlled environments where too many people
have access to the configuration files.  Now maybe you're going to
tell me that the ops guys can't modify the configuration file because
they only have SQL-level access, but then how are they managing to
restart the database?  They need to be able to run pg_ctl *as the
postgres user* to do that, and if they have shell access to that
account, all bets are off.

Sure, you can construct a scenario where this matters.  The ops guys
have sudo postgres pg_ctl access but adminpack isn't installed and
they have no other way to modify the configuration file.  But that's
just bizarre.  And if that's really the environment you have, then you
can install a loadable module that grabs ProcessUtility_hook and uses
it to forbid ALTER SYSTEM on that machine.  Hell, we can ship such a
thing in contrib.  Problem solved.  But it's surely too obscure a
combination of circumstances to justify disabling this by default.

-- 
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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Stephen Frost
Robert,

Was working on replying to this, but got distracted..

* Robert Haas (robertmh...@gmail.com) wrote:
 To be honest, I think the argument that this is dangerous is pretty
 ridiculous.  AFAICS, the only argument anyone's advanced for this
 being dangerous is the theory that you might accidentally put
 something in your postgresql.conf file that makes the server not
 start.  However, the reality is that the superuser has MANY, MANY ways
 of killing the database cluster as things stand.  Consider the
 ever-popular DELETE FROM pg_proc.  

You will not find me argueing to allow that in normal operation, or most
direct-catalog hacking.  I'd be much happier if we had all the ALTER,
etc, options necessary to prevent any need to ever touch the catalogs.
Similairly, it'd be nice to have more permission options which reduce
the need for anyone to be superuser.

 Now, you can argue that people are more likely to render the database
 nonfunctional by changing GUC settings than they are to do it by
 corrupting the system catalogs, but I'm not sure I believe it.  We
 can, after all, validate that any setting a user supplies is a valid
 value for that setting before writing it out to the configuration
 file.  It might still make the system fail to start if - for example -
 it causes too many semaphores to be reserved, or something like that.
 But why should we think that such mistakes will be common?  If
 anything, it sounds less error-prone to me than hand-editing the
 configuration file, where typing something like on_exit_error=maybe
 will make the server fail to start.  We can eliminate that whole
 category of error, at least for people who choose to use the new
 tools.

That category of error is much more likely to get caught by the sysadmin
doing the restart after the update..  ALTER SYSTEM doesn't provide any
way to restart the DB to make sure it worked.  That, in turn, encourages
modification of the config parameters w/o a restart, which is a pretty
bad idea, imv.

 If you're using the term dangerous to refer to a security exposure,
 I concede there is some incremental exposure from allowing this by
 default.  But it's not a terribly large additional exposure.  It's
 already the case that if adminpack is available the super-user can do
 whatever he or she wants, because she or he can write to arbitrary
 files inside the data directory.  

This is only true if -contrib is installed, which a *lot* of admins
refuse to do, specifically because of such insane modules as this.
Therefore, I would argue that it's not nearly as small a change wrt
exposure as you believe.  Strictly speaking, I don't believe it's a huge
security hole or something, but I do think it's going to surprise admins
who aren't following the lists.

 Even if not, for most intents and
 purposes, ALTER DATABASE my_main_database SET whatever = thing is
 functionally equivalent to modifying postgresql.conf.  Some settings
 can't be modified that way, but so what?  AFAICS, about the worst
 thing the user can do is ALTER SYSTEM SET shared_preload_libraries =
 'rootkit'.  

This is assuming a malicious user, and one who has access to the
filesystem, which are pretty big assumptions to be making.  I'm not
trying to make any argument about a malicious user and in general I
don't believe the users of this capability are people who have root
on the box.

 Huh?  The problem with adminpack is that it doesn't let you modify
 individual configuration settings.  All you can do is rewrite an
 entire file.  I guess somebody could write a specialized client that
 just uses that infrastructure to rewrite postgresql.conf.  For all I
 know, someone has.  Even if not, I don't think that you can use that
 to prove that people don't care about this feature.  If nobody cares,
 why are there 400 emails on this topic?!

Addressed this in my reply to Andres.

  Why can't adminpack do that..?
 
 It could.  But it doesn't.  We could improve it to do that, and that
 might be worthwhile, but it still wouldn't be as nice as what's being
 proposed here.

This wouldn't be the first time we've said do it externally and when
we figure out a better answer then we'll put it into core.

 1. disabling the feature by default, and providing no way for it to be
 turned on remotely, and

Yes, that's quite intentional because it would make someone knowledgable
about the *overall system* be aware of this rather than having it sprung
on them when they discover some change has made the database unable to
start.

 2. even when the feature is enabled, only allowing a subset of the
 settings to be changed with it, and
 3. also changing the interpretation of the config files so that we
 have a first-wins rules instead of a last-wins rule.
 
 If we do either #1 or #2, this won't be a plausible substitute for the
 functionality that adminpack offers today.  

While I appreciate that the original thrust behind this may be trying to
replace adminpack, I find that module to be utterly horrendous in 

Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Aug 29, 2013 at 8:07 PM, Stephen Frost sfr...@snowman.net wrote:
  I'm not talking about malicious DBAs but rather a generally
  knowledgable DBA who changed shared_buffers up too high and then leaves on
  vacation, while the OPs guys need to do a database restart for whatever
  reason and then discover it doesn't start.
 
 /me looks at Stephen incredulously.
 
 In the first place, modifying postgresql.conf and not immediately
 restarting the server to test your changes is probably the single most
 basic DBA error imaginable.  

You're not modifying postgresql.conf with ALTER SYSTEM, now are you?
Admins are going to realize the need to restart or at least reload the
service after updating such, but a DBA who has focused mainly on
normalization, query optimization, etc, isn't going to have the same
experience that a sysadmin has.

A DBA updating a GUC, something they are likely to do frequently
day-in and day-out, isn't necessairly going to consider that it's a
reboot-needing change.  Indeed, it's not unreasonable to consider that
we may, some day, actually be able to change or increase
shared_buffers on the fly (or error in the trying); I'd think your
work with the dynamic backends would actually make that likely to
happen sooner rather than later.

 I have a hard time viewing such a person
 as generally knowledgeable.  I hope the DBA in question doesn't have
 access to the switches, because he's probably the sort of guy who
 reassigns switch ports and doesn't type wr m afterwards.

I'd consider the DBAs who I work with as generally knowledgable, and
thankfully also cautious enough to talk to me before making changes, but
they have superuser access (they need to be able to do database
'releases' to our production environments and those scripts need to
change table ownership and do other things which can be annoying to
maintain the permissions for) and certainly understand changing GUCs
(mostly things like work_mem).

 In the second place, the same guy can do the same thing today.  He
 just has to use vi.  In fact, I think this is a pretty common
 failure mode in poorly-controlled environments where too many people
 have access to the configuration files.  Now maybe you're going to
 tell me that the ops guys can't modify the configuration file because
 they only have SQL-level access, but then how are they managing to
 restart the database?  They need to be able to run pg_ctl *as the
 postgres user* to do that, and if they have shell access to that
 account, all bets are off.

You seem to be confused here between the DBA/data team and the OPs or
Infrastructure team.  I'm making a clear distinction between them and
feel quite comfortable with it because it's the environment that I live
in today.  My job, in fact, is exactly to straddle that line and work
with both.

In the above example, the DBA hasn't got root on the box and can't
simply go change or modify postgresql.conf, not even with vi, and is
true for most of the DBAs on my team, even though they have superuser
access in PG.

 Sure, you can construct a scenario where this matters.  The ops guys
 have sudo postgres pg_ctl access but adminpack isn't installed and
 they have no other way to modify the configuration file.  But that's
 just bizarre.  And if that's really the environment you have, then you
 can install a loadable module that grabs ProcessUtility_hook and uses
 it to forbid ALTER SYSTEM on that machine.  Hell, we can ship such a
 thing in contrib.  Problem solved.  But it's surely too obscure a
 combination of circumstances to justify disabling this by default.

It's not the OPs guy that I'm worried about using ALTER SYSTEM- I don't
expect them to have any clue about it or care about it, except where it
can be used to modify things under /etc which they, rightfully, consider
their domain.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-29 Thread Peter Geoghegan
On Thu, Aug 29, 2013 at 9:13 AM, Robert Haas robertmh...@gmail.com wrote:
 I think the goals of this patch should be to (1) let users of other
 clients manipulate the startup configuration just as easily as we can
 already do using pgAdmin and (2) remove some of the concurrency
 hazards that pgAdmin introduces, for example by using locking and
 atomic renames.

+1

-- 
Peter Geoghegan


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-28 Thread Bruce Momjian
On Tue, Aug 27, 2013 at 09:04:00AM +0530, Amit Kapila wrote:
  For my part, I'd honestly rather have the first things found be what's
  picked and later things be ignored.  If you want it controlled by ALTER
  SYSTEM, then don't set it in postgresql.conf.  The problem with that is
  there's no bootstrap mechanism without actually modifying such configs
  or making the configs be in auto.conf to begin with, neither of which is
  ideal, imv.
 
  I really hate the idea that someone could configure 'X' in
  postgresql.conf and because the auto.conf line is later in the file,
  it's able to override the original setting.  Does not strike me as
  intuitive at all.
 
 This is currently how include mechanism works in postgresql.conf,
 changing that for this special case can be costly and moreover the
 specs for this patch were layout from beginning that way.

Agreed.  If you are worried about ALTER SYSTEM changing postgresql.conf
settings, you should move the include_auto line to the top of
postgresql.conf, but I don't think that should be the default.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-28 Thread Stephen Frost
* Bruce Momjian (br...@momjian.us) wrote:
 On Tue, Aug 27, 2013 at 09:04:00AM +0530, Amit Kapila wrote:
   I really hate the idea that someone could configure 'X' in
   postgresql.conf and because the auto.conf line is later in the file,
   it's able to override the original setting.  Does not strike me as
   intuitive at all.
  
  This is currently how include mechanism works in postgresql.conf,
  changing that for this special case can be costly and moreover the
  specs for this patch were layout from beginning that way.
 
 Agreed.  If you are worried about ALTER SYSTEM changing postgresql.conf
 settings, you should move the include_auto line to the top of
 postgresql.conf, but I don't think that should be the default.

While I appreciate that there are bootstrap-type issues with this, I
really don't like this idea of later stuff can just override earlier
stuff.

include files and conf.d-style options are for breaking the config up,
not to allow you to override options because a file came later than an
earlier file.  Our particular implementation of config-file reading
happens to lend itself to later-definition-wins, but that's really
counter-intuitive for anyone unfamiliar with PG, imv.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-28 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes:
 While I appreciate that there are bootstrap-type issues with this, I
 really don't like this idea of later stuff can just override earlier
 stuff.

 include files and conf.d-style options are for breaking the config up,
 not to allow you to override options because a file came later than an
 earlier file.  Our particular implementation of config-file reading
 happens to lend itself to later-definition-wins, but that's really
 counter-intuitive for anyone unfamiliar with PG, imv.

I don't follow this argument at all.  Do you know any software with text
config files that will act differently from this if the same setting is
listed twice?  Last one wins is certainly what I'd expect.

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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-28 Thread Bruce Momjian
On Wed, Aug 28, 2013 at 02:30:41PM -0400, Stephen Frost wrote:
 * Bruce Momjian (br...@momjian.us) wrote:
  On Tue, Aug 27, 2013 at 09:04:00AM +0530, Amit Kapila wrote:
I really hate the idea that someone could configure 'X' in
postgresql.conf and because the auto.conf line is later in the file,
it's able to override the original setting.  Does not strike me as
intuitive at all.
   
   This is currently how include mechanism works in postgresql.conf,
   changing that for this special case can be costly and moreover the
   specs for this patch were layout from beginning that way.
  
  Agreed.  If you are worried about ALTER SYSTEM changing postgresql.conf
  settings, you should move the include_auto line to the top of
  postgresql.conf, but I don't think that should be the default.
 
 While I appreciate that there are bootstrap-type issues with this, I
 really don't like this idea of later stuff can just override earlier
 stuff.
 
 include files and conf.d-style options are for breaking the config up,
 not to allow you to override options because a file came later than an
 earlier file.  Our particular implementation of config-file reading
 happens to lend itself to later-definition-wins, but that's really
 counter-intuitive for anyone unfamiliar with PG, imv.

Agreed, but I think this is a much larger issue than ALTER SYSTEM SET.

I think changing behavior to first-seen would only add to confusion. 
What we really need is a WARNING when a later postgresql.conf setting
overrides an earlier one, and ALTER SYSTEM SET's config file could
behave the same, so you would know right away when you were overriding
something in postgresql.conf that appeared earlier.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-28 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  While I appreciate that there are bootstrap-type issues with this, I
  really don't like this idea of later stuff can just override earlier
  stuff.
 
  include files and conf.d-style options are for breaking the config up,
  not to allow you to override options because a file came later than an
  earlier file.  Our particular implementation of config-file reading
  happens to lend itself to later-definition-wins, but that's really
  counter-intuitive for anyone unfamiliar with PG, imv.
 
 I don't follow this argument at all.  Do you know any software with text
 config files that will act differently from this if the same setting is
 listed twice?  Last one wins is certainly what I'd expect.

Have you tried having the same mount specified in multiple files in
fstab.d..?  Also, aiui (not a big exim fan personally), duplicate
definitions in an exim4/conf.d would result in an error.  Playing around
in apache2/sites-enabled, it appears to be first wins wrt virtual
hosts.

There's a number of cases where only a single value is being set and
subseqeunt files are 'additive' (eg: ld.so.conf.d), so they don't have
this issue.  Similar to that are script directories, which simply run a
set of scripts in the $DIR.d and, as it's the scripts themselves which
are the 'parameters', and being files in a directory, you can't
duplicate them.  Others (eg: pam.d) define the file name to be an
enclosing context, again preventing duplication by using the filename
itself.

There are counter-examples also; sysctl.d will replace what's already
been set, so perhaps it simply depends on an individual's experience.
For my part, I'd much prefer an error or warning saying you've got
duplicate definitions of X than a last-wins approach, though perhaps
that's just because I like things to be very explicit and clear.
Allowing multiple definitions of the same parameter to be valid isn't
'clear' to me.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-28 Thread Stephen Frost
* Bruce Momjian (br...@momjian.us) wrote:
 Agreed, but I think this is a much larger issue than ALTER SYSTEM SET.

Yeah, true.

 I think changing behavior to first-seen would only add to confusion. 
 What we really need is a WARNING when a later postgresql.conf setting
 overrides an earlier one, and ALTER SYSTEM SET's config file could
 behave the same, so you would know right away when you were overriding
 something in postgresql.conf that appeared earlier.

A warning would be nice.  If only everyone read the log files. :/

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-28 Thread Bruce Momjian
On Wed, Aug 28, 2013 at 03:15:14PM -0400, Stephen Frost wrote:
 * Bruce Momjian (br...@momjian.us) wrote:
  Agreed, but I think this is a much larger issue than ALTER SYSTEM SET.
 
 Yeah, true.
 
  I think changing behavior to first-seen would only add to confusion. 
  What we really need is a WARNING when a later postgresql.conf setting
  overrides an earlier one, and ALTER SYSTEM SET's config file could
  behave the same, so you would know right away when you were overriding
  something in postgresql.conf that appeared earlier.
 
 A warning would be nice.  If only everyone read the log files. :/

I would expect ALTER SYSTEM SET to return a WARNING in such cases too.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-28 Thread Alvaro Herrera
Stephen Frost escribió:

 There are counter-examples also; sysctl.d will replace what's already
 been set, so perhaps it simply depends on an individual's experience.
 For my part, I'd much prefer an error or warning saying you've got
 duplicate definitions of X than a last-wins approach, though perhaps
 that's just because I like things to be very explicit and clear.
 Allowing multiple definitions of the same parameter to be valid isn't
 'clear' to me.

One of the elements that had to be in place for this whole thing of
multiple configuration files to be allowed was for pg_settings to
include precise information about, for each setting, where is its value
coming from.  I doubt any of the systems you mentioned have comparable
functionality.  If the admin has trouble figuring it out, he needs only
look into that view.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-26 Thread Stephen Frost
Martijn,

* Martijn van Oosterhout (klep...@svana.org) wrote:
 Note, my whole purpose for suggesting something like:
 
 include_auto_conf_filepostgresql.auto.conf
 
 is because I want the file location to be configurable. If I put in my
 configuration:
 
 include_auto_conf_file/etc/postgresql/9.4/postgresql.auto.conf

I'm not following the use-case here.

Why would it make sense for a file which is entirely managed by PG
automatically to be in /etc?  Of course, by the same token you might ask
why it makes sense to let the user pick it at all, which holds some
merit- but I liked your idea because then an admin doesn't have to go
looking for the file but instead knows where it is.  Perhaps comments in
the file stating where the auto.conf lives would be sufficient, but
those are too often nuked. :(

 it better put the options there. And if I comment the line out ALTER
 SYSTEM should stop working.  If I put it at the beginning of the config
 then any other option in the file will override it (which we can detect
 and warn about).  If I put it at the end of the file, ALTER SYSTEM
 overrides any statically configured options.
 
 And I can imagine hosting providers putting the configuration for
 memory usage, shared library directories and other such options after,
 and options like cpu_cost, enable_merge_join, etc before.  That way
 they have fine grained control over which options the user can set and
 which not.

For my part, I'd honestly rather have the first things found be what's
picked and later things be ignored.  If you want it controlled by ALTER
SYSTEM, then don't set it in postgresql.conf.  The problem with that is
there's no bootstrap mechanism without actually modifying such configs
or making the configs be in auto.conf to begin with, neither of which is
ideal, imv.

I really hate the idea that someone could configure 'X' in
postgresql.conf and because the auto.conf line is later in the file,
it's able to override the original setting.  Does not strike me as
intuitive at all.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-26 Thread Amit Kapila
On Mon, Aug 26, 2013 at 10:50 PM, Stephen Frost sfr...@snowman.net wrote:
 Martijn,

 * Martijn van Oosterhout (klep...@svana.org) wrote:
 Note, my whole purpose for suggesting something like:

 include_auto_conf_filepostgresql.auto.conf

 is because I want the file location to be configurable. If I put in my
 configuration:

 include_auto_conf_file/etc/postgresql/9.4/postgresql.auto.conf

 I'm not following the use-case here.

 Why would it make sense for a file which is entirely managed by PG
 automatically to be in /etc?  Of course, by the same token you might ask
 why it makes sense to let the user pick it at all, which holds some
 merit- but I liked your idea because then an admin doesn't have to go
 looking for the file but instead knows where it is.  Perhaps comments in
 the file stating where the auto.conf lives would be sufficient, but
 those are too often nuked. :(

 it better put the options there. And if I comment the line out ALTER
 SYSTEM should stop working.  If I put it at the beginning of the config
 then any other option in the file will override it (which we can detect
 and warn about).  If I put it at the end of the file, ALTER SYSTEM
 overrides any statically configured options.

 And I can imagine hosting providers putting the configuration for
 memory usage, shared library directories and other such options after,
 and options like cpu_cost, enable_merge_join, etc before.  That way
 they have fine grained control over which options the user can set and
 which not.

 For my part, I'd honestly rather have the first things found be what's
 picked and later things be ignored.  If you want it controlled by ALTER
 SYSTEM, then don't set it in postgresql.conf.  The problem with that is
 there's no bootstrap mechanism without actually modifying such configs
 or making the configs be in auto.conf to begin with, neither of which is
 ideal, imv.

 I really hate the idea that someone could configure 'X' in
 postgresql.conf and because the auto.conf line is later in the file,
 it's able to override the original setting.  Does not strike me as
 intuitive at all.

This is currently how include mechanism works in postgresql.conf,
changing that for this special case can be costly and moreover the
specs for this patch were layout from beginning that way.

However, we can have some mechanism for appending values (if feasible)
as suggested in mail below, after initial patch is done
http://www.postgresql.org/message-id/52015414.9060...@cybertec.at


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-24 Thread Martijn van Oosterhout
On Fri, Aug 23, 2013 at 06:41:04PM +0530, Amit Kapila wrote:
  Not with this proposal...  If it's fixed then it makes no sense to make it
  look like it can be modified.
 
This proposal is to have a special include which user can only use
 for enable/disable
which means he can remove symbol '#' or add '#'.
We cannot stop user from changing file name or add some different
 location, but that can lead to problems.
We can have a note on top of this include indicating it is only for
 enable/disable.

Note, my whole purpose for suggesting something like:

include_auto_conf_filepostgresql.auto.conf

is because I want the file location to be configurable. If I put in my
configuration:

include_auto_conf_file/etc/postgresql/9.4/postgresql.auto.conf

it better put the options there. And if I comment the line out ALTER
SYSTEM should stop working.  If I put it at the beginning of the config
then any other option in the file will override it (which we can detect
and warn about).  If I put it at the end of the file, ALTER SYSTEM
overrides any statically configured options.

And I can imagine hosting providers putting the configuration for
memory usage, shared library directories and other such options after,
and options like cpu_cost, enable_merge_join, etc before.  That way
they have fine grained control over which options the user can set and
which not.

I think if we add more meaning to it, like allow user to change it,
 handling and defining of that will be bit complex.

Letting the user configure the location seems like common curtesy. Note
this line isn't in itself a GUC, so you can't configure it via ALTER
SYSTEM.

Have a nice day,
-- 
Martijn van Oosterhout   klep...@svana.org   http://svana.org/kleptog/
 He who writes carelessly confesses thereby at the very outset that he does
 not attach much importance to his own thoughts.
   -- Arthur Schopenhauer


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-24 Thread Amit Kapila
On Sat, Aug 24, 2013 at 6:08 PM, Martijn van Oosterhout
klep...@svana.org wrote:
 On Fri, Aug 23, 2013 at 06:41:04PM +0530, Amit Kapila wrote:
  Not with this proposal...  If it's fixed then it makes no sense to make it
  look like it can be modified.

This proposal is to have a special include which user can only use
 for enable/disable
which means he can remove symbol '#' or add '#'.
We cannot stop user from changing file name or add some different
 location, but that can lead to problems.
We can have a note on top of this include indicating it is only for
 enable/disable.

 Note, my whole purpose for suggesting something like:

 include_auto_conf_filepostgresql.auto.conf

 is because I want the file location to be configurable. If I put in my
 configuration:

 include_auto_conf_file/etc/postgresql/9.4/postgresql.auto.conf

 it better put the options there. And if I comment the line out ALTER
 SYSTEM should stop working.  If I put it at the beginning of the config
 then any other option in the file will override it (which we can detect
 and warn about).  If I put it at the end of the file, ALTER SYSTEM
 overrides any statically configured options.

 And I can imagine hosting providers putting the configuration for
 memory usage, shared library directories and other such options after,
 and options like cpu_cost, enable_merge_join, etc before.  That way
 they have fine grained control over which options the user can set and
 which not.

   Thanks for your suggestion.
   Above usecase can be achieved even if the file is not configurable.

I think if we add more meaning to it, like allow user to change it,
 handling and defining of that will be bit complex.

 Letting the user configure the location seems like common curtesy. Note
 this line isn't in itself a GUC, so you can't configure it via ALTER
 SYSTEM.

In general yes it is better to give control to user for configuring
the location, but as this file will
be used for internal purpose and will be modified by system and not by
user, so it is better to
be in data directory. The similar thing was discussed previously on
this thread and it is suggested
to have this file in data directory.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-23 Thread Stephen Frost
* Amit Kapila (amit.kapil...@gmail.com) wrote:
 On Thu, Aug 22, 2013 at 6:06 PM, Stephen Frost sfr...@snowman.net wrote:
  * Amit Kapila (amit.kapil...@gmail.com) wrote:
  Enable/Disable reading of auto file
  -
  a. Have a new include in postresql.conf
  #include_auto_conf_filepostgresql.auto.conf
  as it is a special include, we can read this file relative to data
  directory.
 
  Enable/Disable Alter System command
  ---
  This can be achieved in 3 ways:
  a. Check before executing Alter System if include directive is
  disabled, then just issue a warning to user and proceed with command.
  b. Check before executing Alter System if include directive is
  disabled, then just issue an error and stop.
 
  It doesn't make sense for it to be a 'warning' with this- the
  parameter specifies the file to use.  If you don't know what file to
  use, how you can possibly do anything but return an error?
 
As the file and location are fixed, we can go-ahead and write to
 it, but I think now we are deciding
if someone disables include dir, then we can just disable Alter
 System, so it is better to return error in such
situation.

It wouldn't be fixed with this approach.

  Note that I *like* that about this approach.
 
  There are a few other considerations with this-
 
  - What should the default be?  (Still thinking 'off' myself)
  default 'off' is a safe option, as it won't allow users to make
 any change to parameter values until/unless they
  read from manual, how to use it and what can go wrong, on the
 other side it will be bit hassle for user to use this
  command. I think 'on' would be better.

Yeah, no, I still think 'off' would be best for this particular option.

  - What happens if the user specifies 'postgresql.conf'?  I'm thinking we
would disallow such insanity (as that's what it is, imv..) by having
an identifier in the file that this is the PG auto conf file.
   I think we can detect by name and give error.
  - Should we have such an identifier in auto.conf to indicate that we
created it, to prevent the user from setting it to something they
shouldn't?
  I think if user plays with this file manually, it can lead to
 problems, that's why earlier we have
  decided to keep a note on top of file which will indicate, do not
 edit this file manually.
  I believe that should be sufficient.

I agree that having such a disclaimer at the top of the file is a good
idea.  I'm not completely convinced that's sufficient but it's certainly
better than nothing.

  - What's the bootstrap mode; iow, if a user enables the option but the
file doesn't exist, what do we do?  With this approach, I'd be
inclined to say we simply create it and put the marker to indicate
it's our file.
 
  Alter System will create the file if doesn't exist.

... Only if it's enabled though.

  - Should we allow it to be outside of the data dir?  We could simply log
an error and ignore the parameter if it's more than a simple filename.
 
  This should be an error, the file location and name will be fixed.

Not with this proposal...  If it's fixed then it makes no sense to make it
look like it can be modified.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-23 Thread Amit Kapila
On Fri, Aug 23, 2013 at 6:01 PM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
 On Thu, Aug 22, 2013 at 6:06 PM, Stephen Frost sfr...@snowman.net wrote:
  * Amit Kapila (amit.kapil...@gmail.com) wrote:
  Enable/Disable reading of auto file
  -
  a. Have a new include in postresql.conf
  #include_auto_conf_filepostgresql.auto.conf
  as it is a special include, we can read this file relative to data
  directory.
 
  Enable/Disable Alter System command
  ---
  This can be achieved in 3 ways:
  a. Check before executing Alter System if include directive is
  disabled, then just issue a warning to user and proceed with command.
  b. Check before executing Alter System if include directive is
  disabled, then just issue an error and stop.
 
  It doesn't make sense for it to be a 'warning' with this- the
  parameter specifies the file to use.  If you don't know what file to
  use, how you can possibly do anything but return an error?

As the file and location are fixed, we can go-ahead and write to
 it, but I think now we are deciding
if someone disables include dir, then we can just disable Alter
 System, so it is better to return error in such
situation.

 It wouldn't be fixed with this approach.

  Note that I *like* that about this approach.
 
  There are a few other considerations with this-
 
  - What should the default be?  (Still thinking 'off' myself)
  default 'off' is a safe option, as it won't allow users to make
 any change to parameter values until/unless they
  read from manual, how to use it and what can go wrong, on the
 other side it will be bit hassle for user to use this
  command. I think 'on' would be better.

 Yeah, no, I still think 'off' would be best for this particular option.

  - What happens if the user specifies 'postgresql.conf'?  I'm thinking we
would disallow such insanity (as that's what it is, imv..) by having
an identifier in the file that this is the PG auto conf file.
   I think we can detect by name and give error.
  - Should we have such an identifier in auto.conf to indicate that we
created it, to prevent the user from setting it to something they
shouldn't?
  I think if user plays with this file manually, it can lead to
 problems, that's why earlier we have
  decided to keep a note on top of file which will indicate, do not
 edit this file manually.
  I believe that should be sufficient.

 I agree that having such a disclaimer at the top of the file is a good
 idea.  I'm not completely convinced that's sufficient but it's certainly
 better than nothing.

  - What's the bootstrap mode; iow, if a user enables the option but the
file doesn't exist, what do we do?  With this approach, I'd be
inclined to say we simply create it and put the marker to indicate
it's our file.

  Alter System will create the file if doesn't exist.

 ... Only if it's enabled though.

Yes.

  - Should we allow it to be outside of the data dir?  We could simply log
an error and ignore the parameter if it's more than a simple filename.

  This should be an error, the file location and name will be fixed.

 Not with this proposal...  If it's fixed then it makes no sense to make it
 look like it can be modified.

   This proposal is to have a special include which user can only use
for enable/disable
   which means he can remove symbol '#' or add '#'.
   We cannot stop user from changing file name or add some different
location, but that can lead to problems.
   We can have a note on top of this include indicating it is only for
enable/disable.

   I think if we add more meaning to it, like allow user to change it,
handling and defining of that will be bit complex.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-22 Thread Stephen Frost
* Amit Kapila (amit.kapil...@gmail.com) wrote:
This can resolve the problem of whether to read auto file rather
 cleanly, so the idea is:
 
 Enable/Disable reading of auto file
 -
 a. Have a new include in postresql.conf
 #include_auto_conf_filepostgresql.auto.conf
 as it is a special include, we can read this file relative to data
 directory.
 
 Enable/Disable Alter System command
 ---
 This can be achieved in 3 ways:
 a. Check before executing Alter System if include directive is
 disabled, then just issue a warning to user and proceed with command.
 b. Check before executing Alter System if include directive is
 disabled, then just issue an error and stop.

It doesn't make sense for it to be a 'warning' with this- the
parameter specifies the file to use.  If you don't know what file to
use, how you can possibly do anything but return an error?

Note that I *like* that about this approach.

There are a few other considerations with this-

- What should the default be?  (Still thinking 'off' myself)
- What happens if the user specifies 'postgresql.conf'?  I'm thinking we
  would disallow such insanity (as that's what it is, imv..) by having
  an identifier in the file that this is the PG auto conf file.
- Should we have such an identifier in auto.conf to indicate that we
  created it, to prevent the user from setting it to something they
  shouldn't?
- What's the bootstrap mode; iow, if a user enables the option but the
  file doesn't exist, what do we do?  With this approach, I'd be
  inclined to say we simply create it and put the marker to indicate
  it's our file.
- Should we allow it to be outside of the data dir?  We could simply log
  an error and ignore the parameter if it's more than a simple filename.

There are probably other considerations also..

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-22 Thread Bruce Momjian
On Thu, Aug 22, 2013 at 08:36:37AM -0400, Stephen Frost wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
 This can resolve the problem of whether to read auto file rather
  cleanly, so the idea is:
  
  Enable/Disable reading of auto file
  -
  a. Have a new include in postresql.conf
  #include_auto_conf_filepostgresql.auto.conf
  as it is a special include, we can read this file relative to data
  directory.

The big advantage of using 'include_auto_conf_file' and not simply
'include' is that we can issue an error from ALTER SYSTEM SET if that is
not set.

  Enable/Disable Alter System command
  ---
  This can be achieved in 3 ways:
  a. Check before executing Alter System if include directive is
  disabled, then just issue a warning to user and proceed with command.
  b. Check before executing Alter System if include directive is
  disabled, then just issue an error and stop.
 
 It doesn't make sense for it to be a 'warning' with this- the
 parameter specifies the file to use.  If you don't know what file to
 use, how you can possibly do anything but return an error?

Agreed.  No sense in allowing users to add things to the 'auto' file
when the auto file is inactive.

 Note that I *like* that about this approach.
 
 There are a few other considerations with this-
 
 - What should the default be?  (Still thinking 'off' myself)

Probably, but we might need to wait until we have a final API for a
decision on that.

 - What happens if the user specifies 'postgresql.conf'?  I'm thinking we
   would disallow such insanity (as that's what it is, imv..) by having
   an identifier in the file that this is the PG auto conf file.

I am thinking they can't include a value equal to 'config_file', which
is normally postgresql.conf.  I am not a big fan of looking for special
text in files.  This might be complex to check, though, because of path
changes --- we might just disallow the basement from matching the
basename of config_file.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-22 Thread Amit Kapila
On Thu, Aug 22, 2013 at 6:06 PM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
This can resolve the problem of whether to read auto file rather
 cleanly, so the idea is:

 Enable/Disable reading of auto file
 -
 a. Have a new include in postresql.conf
 #include_auto_conf_filepostgresql.auto.conf
 as it is a special include, we can read this file relative to data
 directory.

 Enable/Disable Alter System command
 ---
 This can be achieved in 3 ways:
 a. Check before executing Alter System if include directive is
 disabled, then just issue a warning to user and proceed with command.
 b. Check before executing Alter System if include directive is
 disabled, then just issue an error and stop.

 It doesn't make sense for it to be a 'warning' with this- the
 parameter specifies the file to use.  If you don't know what file to
 use, how you can possibly do anything but return an error?

   As the file and location are fixed, we can go-ahead and write to
it, but I think now we are deciding
   if someone disables include dir, then we can just disable Alter
System, so it is better to return error in such
   situation.

 Note that I *like* that about this approach.

 There are a few other considerations with this-

 - What should the default be?  (Still thinking 'off' myself)
 default 'off' is a safe option, as it won't allow users to make
any change to parameter values until/unless they
 read from manual, how to use it and what can go wrong, on the
other side it will be bit hassle for user to use this
 command. I think 'on' would be better.
 - What happens if the user specifies 'postgresql.conf'?  I'm thinking we
   would disallow such insanity (as that's what it is, imv..) by having
   an identifier in the file that this is the PG auto conf file.
  I think we can detect by name and give error.
 - Should we have such an identifier in auto.conf to indicate that we
   created it, to prevent the user from setting it to something they
   shouldn't?
 I think if user plays with this file manually, it can lead to
problems, that's why earlier we have
 decided to keep a note on top of file which will indicate, do not
edit this file manually.
 I believe that should be sufficient.

 - What's the bootstrap mode; iow, if a user enables the option but the
   file doesn't exist, what do we do?  With this approach, I'd be
   inclined to say we simply create it and put the marker to indicate
   it's our file.

 Alter System will create the file if doesn't exist.

 - Should we allow it to be outside of the data dir?  We could simply log
   an error and ignore the parameter if it's more than a simple filename.

 This should be an error, the file location and name will be fixed.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-22 Thread Amit Kapila
On Thu, Aug 22, 2013 at 9:34 PM, Bruce Momjian br...@momjian.us wrote:
 On Thu, Aug 22, 2013 at 08:36:37AM -0400, Stephen Frost wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
 This can resolve the problem of whether to read auto file rather
  cleanly, so the idea is:
 
  Enable/Disable reading of auto file
  -
  a. Have a new include in postresql.conf
  #include_auto_conf_filepostgresql.auto.conf
  as it is a special include, we can read this file relative to data
  directory.

 The big advantage of using 'include_auto_conf_file' and not simply
 'include' is that we can issue an error from ALTER SYSTEM SET if that is
 not set.

  Enable/Disable Alter System command
  ---
  This can be achieved in 3 ways:
  a. Check before executing Alter System if include directive is
  disabled, then just issue a warning to user and proceed with command.
  b. Check before executing Alter System if include directive is
  disabled, then just issue an error and stop.

 It doesn't make sense for it to be a 'warning' with this- the
 parameter specifies the file to use.  If you don't know what file to
 use, how you can possibly do anything but return an error?

 Agreed.  No sense in allowing users to add things to the 'auto' file
 when the auto file is inactive.

 Note that I *like* that about this approach.

 There are a few other considerations with this-

 - What should the default be?  (Still thinking 'off' myself)

 Probably, but we might need to wait until we have a final API for a
 decision on that.

 - What happens if the user specifies 'postgresql.conf'?  I'm thinking we
   would disallow such insanity (as that's what it is, imv..) by having
   an identifier in the file that this is the PG auto conf file.

 I am thinking they can't include a value equal to 'config_file', which
 is normally postgresql.conf.  I am not a big fan of looking for special
 text in files.  This might be complex to check, though, because of path
 changes --- we might just disallow the basement from matching the
 basename of config_file.

   Right, I also think that as file and location are fixed, so it can
be detected with name.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-21 Thread Stephen Frost
Martijn,

* Martijn van Oosterhout (klep...@svana.org) wrote:
 ISTM you want some kind of hybrid setting like:
 
 #include_system auto.conf
 
 which simultaneously does three things:
 
 1. Sets the enable_alter_system flag
 2. Indicates the file to use
 3. Indicates the priority of the setting re other settings.
 
 Comment it out, ALTER SYSTEM stop working. Put it back and it's
 immediately clear what it means. And the user can control where the
 settings go.

Yeah, that's certainly an interesting idea.  I might call it
'auto_conf_file auto.conf' to avoid the '#include' concern and to
perhaps clarify that it's more than just a regular 'include'.

 Syntax is a bit fugly though.

Agreed.

Thanks,

Stephen
(who is still unhappy about the GUC-specific handling for 
relative
paths in postgresql.conf)


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-21 Thread Amit Kapila
On Wed, Aug 21, 2013 at 8:22 PM, Stephen Frost sfr...@snowman.net wrote:
 Martijn,

 * Martijn van Oosterhout (klep...@svana.org) wrote:
 ISTM you want some kind of hybrid setting like:

 #include_system auto.conf

 which simultaneously does three things:

 1. Sets the enable_alter_system flag
 2. Indicates the file to use
 3. Indicates the priority of the setting re other settings.

 Comment it out, ALTER SYSTEM stop working. Put it back and it's
 immediately clear what it means. And the user can control where the
 settings go.

 Yeah, that's certainly an interesting idea.  I might call it
 'auto_conf_file auto.conf' to avoid the '#include' concern and to
 perhaps clarify that it's more than just a regular 'include'.

   This can resolve the problem of whether to read auto file rather
cleanly, so the idea is:

Enable/Disable reading of auto file
-
a. Have a new include in postresql.conf
#include_auto_conf_filepostgresql.auto.conf
as it is a special include, we can read this file relative to data
directory.

Enable/Disable Alter System command
---
This can be achieved in 3 ways:
a. Check before executing Alter System if include directive is
disabled, then just issue a warning to user and proceed with command.
b. Check before executing Alter System if include directive is
disabled, then just issue an error and stop.
c. Have a new guc enable_alter_system which will behave as described
in my previous mail and below:
   1. enable_alter_system a new GUC whose default value =off.
2. Alter System will check this variable and return error (not
allowed), if this parameter is off.
3. Now if user enables include directive in postgresql.conf, it will
enable Alter System as value of enable_alter_system is on.
4. User can run Alter System command to disable Alter System
enable_alter_system = off.
Now even though include directive is enabled, but new Alter System
commands will not work, however
existing parameter's take into effect on restart/sighup.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Amit Kapila (amit.kapil...@gmail.com) wrote:
 Okay, let us decide how things will be if we disable by default:
1. initdb won't create any empty auto file, rather the file will be
 created first time user uses ALTER SYSTEM.

I'm not particularly set on this..  Why not create the file initially?

2. Alter system should issue warning, if it detects that feature is
 disabled (for this we need to error detection code
during parsing of postgresql.conf as it was previously)

I agree that it should complain through a warning or perhaps an error
(to be honest, I like error more, but there may be good reasons against
that).

3. postgresql.conf will contain include directive in below form:
#include = 'postgresql.auto.conf'
Whenever user wants to use Alter System, he needs to enable it
 after first time using ALTER SYSTEM.

That I don't like..  You should be able to enable it and have things
work without having to run ALTER SYSTEM first..

 One question here, if somebody just enables it without using ALTER
 SYSTEM, should it throw any error on not finding auto file or just
 ignore it?

I'd prefer that it error if it can't find an included file.

  What about include directives?
 
 Sorry, I didn't get which parameters you are referring by include directives?

Literally, the above 'include' in postgresql.conf.  Would that only be
dealt with as a parse-time thing, or should it be more like a GUC which
could then be set through ALTER SYSTEM?

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Amit Kapila escribió:
 3. postgresql.conf will contain include directive in below form:
 #include = 'postgresql.auto.conf'
 Whenever user wants to use Alter System, he needs to enable it
  after first time using ALTER SYSTEM.
 
 This seems wrong to me.  If the auto file is read by an include line in
 postgresql.conf, what is its priority w.r.t. files placed in an
 hypothetical conf.d directory?  

This could be handled by a simple ordering with last one wins.  I
don't particularly like that though..  My gut feeling is that I'd like
something to complain if there's more than one value set for the same
GUC..

 Hopefully snippets put in conf.d/ by
 puppet/chef will override the settings in postgresql.conf (i.e. conf.d/
 should be processed after postgresql.conf, not before); and hopefully
 ALTER SYSTEM will in turn override conf.d.  I see no way to have ALTER
 SYSTEM handled by an include line, yet still have it override conf.d.

With includedir and include directives, and postgresql.conf setting a
defined ordering, with last-wins, you could simply have the 'includedir'
for conf.d come before the 'include' for auto.conf.

 If we want to make ALTER SYSTEM disable-able from postgresql.conf, I
 think it should be an explicit option, something like
 enable_alter_system = on
 or something like that.

I really don't like this approach- I'd much rather we have a general
include mechanism than special alter-system GUCs.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Amit Kapila
On Tue, Aug 20, 2013 at 6:21 PM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
 Okay, let us decide how things will be if we disable by default:
1. initdb won't create any empty auto file, rather the file will be
 created first time user uses ALTER SYSTEM.

 I'm not particularly set on this..  Why not create the file initially?
   If by default this feature needs to be disabled, then it is not
must to have at initdb time.
   OTOH if we want to enable it by default, then we need to get this
created at initdb time else it will give error during
   system startup (an error can occur during startup when it will
parse postgresql.conf and didn't find the file
   mentioned in include directive).

   Also you mentioned below line upthread which I understood as you
don't like idea of creating empty file at initdb
   time:
   If it's enabled by default, then we need to ship an 'auto' file which is
empty by default...  I don't particularly like that


2. Alter system should issue warning, if it detects that feature is
 disabled (for this we need to error detection code
during parsing of postgresql.conf as it was previously)

 I agree that it should complain through a warning or perhaps an error
 (to be honest, I like error more, but there may be good reasons against
 that).

3. postgresql.conf will contain include directive in below form:
#include = 'postgresql.auto.conf'
Whenever user wants to use Alter System, he needs to enable it
 after first time using ALTER SYSTEM.

 That I don't like..  You should be able to enable it and have things
 work without having to run ALTER SYSTEM first..

   This was actually linked to point 1 mentioned above, if we create
empty file at initdb time, then there should not be
   any problem.

   One other option to disable as suggested by Alvaro is have another
config parameter to enable/disable Alter
   System, if we can do that way, the solution will be much neater.

 One question here, if somebody just enables it without using ALTER
 SYSTEM, should it throw any error on not finding auto file or just
 ignore it?

 I'd prefer that it error if it can't find an included file.

  What about include directives?

 Sorry, I didn't get which parameters you are referring by include directives?

 Literally, the above 'include' in postgresql.conf.  Would that only be
 dealt with as a parse-time thing, or should it be more like a GUC which
 could then be set through ALTER SYSTEM?

 I think if we choose to use include directive as a way to
enable/disable this feature, it will not be good to allow change of
this parameter with Alter System.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Amit Kapila (amit.kapil...@gmail.com) wrote:
 On Tue, Aug 20, 2013 at 6:21 PM, Stephen Frost sfr...@snowman.net wrote:
  I'm not particularly set on this..  Why not create the file initially?
If by default this feature needs to be disabled, then it is not
 must to have at initdb time.

I don't see how the two are related.  You could never use two-phase
commit (could even disable it), yet we still create things in the data
directory to support it.  Having a file in the data directory which
isn't used by default isn't that big a deal, imv.

Also you mentioned below line upthread which I understood as you
 don't like idea of creating empty file at initdb
time:
If it's enabled by default, then we need to ship an 'auto' file which is
 empty by default...  I don't particularly like that

What I didn't like was having an empty file be accepted as 'valid', but
you need to have some kind of bootstrap mechanism.  Telling users run
this command and then ignore the warning is certainly bad.  A better
option might be to have a *non-empty* auto.conf be generated, where all
it has in it is some kind of identifier, perhaps even just
enable_alter_system = on which we could then key off of to see if
ALTER SYSTEM has been set up to be allowed or not.

  I think if we choose to use include directive as a way to
 enable/disable this feature, it will not be good to allow change of
 this parameter with Alter System.

I agree, but then we need to add it to the list of things ALTER SYSTEM
can't modify (if the include is implemented as a GUC, that is; I've not
looked).

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Amit Kapila
On Tue, Aug 20, 2013 at 6:55 PM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
 On Tue, Aug 20, 2013 at 6:21 PM, Stephen Frost sfr...@snowman.net wrote:
  I'm not particularly set on this..  Why not create the file initially?
If by default this feature needs to be disabled, then it is not
 must to have at initdb time.

 I don't see how the two are related.  You could never use two-phase
 commit (could even disable it), yet we still create things in the data
 directory to support it.  Having a file in the data directory which
 isn't used by default isn't that big a deal, imv.

   Yes, it can be done, what I wanted to say it is not a must,
rather a good to have thing.

Also you mentioned below line upthread which I understood as you
 don't like idea of creating empty file at initdb
time:
If it's enabled by default, then we need to ship an 'auto' file which is
 empty by default...  I don't particularly like that

 What I didn't like was having an empty file be accepted as 'valid', but
 you need to have some kind of bootstrap mechanism.  Telling users run
 this command and then ignore the warning is certainly bad.  A better
 option might be to have a *non-empty* auto.conf be generated, where all
 it has in it is some kind of identifier, perhaps even just
 enable_alter_system = on which we could then key off of to see if
 ALTER SYSTEM has been set up to be allowed or not.

Wouldn't it be complicated to handle this way rather then by having
include directive.
If include directive is enabled then the autofile will be read else no
need to read it.

If we want to have separate identifier to judge ALTER SYSTEM is enable
or not, then it is better to go with a new GUC.

  I think if we choose to use include directive as a way to
 enable/disable this feature, it will not be good to allow change of
 this parameter with Alter System.

 I agree, but then we need to add it to the list of things ALTER SYSTEM
 can't modify (if the include is implemented as a GUC, that is; I've not
 looked).

   I have checked and it seems to be just parse time thing.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Amit Kapila (amit.kapil...@gmail.com) wrote:
 Wouldn't it be complicated to handle this way rather then by having
 include directive.

You misunderstood.  I was suggesting a setup along these lines:

postgresql.conf:
  # include = 'auto.conf'  # Disabled by default

auto.conf:
  
  # MANAGED BY ALTER SYSTEM -- DO NOT MODIFY BY HAND
  

  # auto.conf not processed unless included by postgresql.conf
  # If included by postgresql.conf, then this will turn on the
  # ALTER SYSTEM command.
  enable_alter_system = on 

Which would give us the ability to independently turn on/off ALTER
SYSTEM from including auto.conf.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Amit Kapila
On Tue, Aug 20, 2013 at 7:51 PM, Stephen Frost sfr...@snowman.net wrote:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
 Wouldn't it be complicated to handle this way rather then by having
 include directive.

 You misunderstood.  I was suggesting a setup along these lines:

 postgresql.conf:
   # include = 'auto.conf'  # Disabled by default

 auto.conf:
   
   # MANAGED BY ALTER SYSTEM -- DO NOT MODIFY BY HAND
   

   # auto.conf not processed unless included by postgresql.conf
   # If included by postgresql.conf, then this will turn on the
   # ALTER SYSTEM command.
   enable_alter_system = on

 Which would give us the ability to independently turn on/off ALTER
 SYSTEM from including auto.conf.

So let me try to explain what I understood from above:

1. enable_alter_system a new GUC whose default value =off.
2. Alter System will check this variable and return error (not
allowed), if this parameter is off.
3. Now if user enables include directive in postgresql.conf, it will
enable Alter System as value of
enable_alter_system is on.
4. User can run Alter System command to disable Alter System
enable_alter_system = off.
Now even though include directive is enabled, but new Alter System
commands will not work, however
existing parameter's take into effect on restart/sighup.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Amit Kapila (amit.kapil...@gmail.com) wrote:
 So let me try to explain what I understood from above:
 
 1. enable_alter_system a new GUC whose default value =off.
 2. Alter System will check this variable and return error (not
 allowed), if this parameter is off.
 3. Now if user enables include directive in postgresql.conf, it will
 enable Alter System as value of
 enable_alter_system is on.
 4. User can run Alter System command to disable Alter System
 enable_alter_system = off.
 Now even though include directive is enabled, but new Alter System
 commands will not work, however
 existing parameter's take into effect on restart/sighup.

Yes.  Not sure that it'd be terribly likely for a user to do that, but
if they do it, so be it.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Alvaro Herrera
Stephen Frost escribió:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:

  Hopefully snippets put in conf.d/ by
  puppet/chef will override the settings in postgresql.conf (i.e. conf.d/
  should be processed after postgresql.conf, not before); and hopefully
  ALTER SYSTEM will in turn override conf.d.  I see no way to have ALTER
  SYSTEM handled by an include line, yet still have it override conf.d.
 
 With includedir and include directives, and postgresql.conf setting a
 defined ordering, with last-wins, you could simply have the 'includedir'
 for conf.d come before the 'include' for auto.conf.

Uh, I was thinking that conf.d would be read by the system
automatically, not because of an includedir line in postgresql.conf.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost escribió:
  With includedir and include directives, and postgresql.conf setting a
  defined ordering, with last-wins, you could simply have the 'includedir'
  for conf.d come before the 'include' for auto.conf.
 
 Uh, I was thinking that conf.d would be read by the system
 automatically, not because of an includedir line in postgresql.conf.

I'd much rather have an includedir directive than some hard-coded or
command-line option to read the directory..  The directory should live
in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives..

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Alvaro Herrera
Stephen Frost escribió:
 * Amit Kapila (amit.kapil...@gmail.com) wrote:
  So let me try to explain what I understood from above:
  
  1. enable_alter_system a new GUC whose default value =off.
  2. Alter System will check this variable and return error (not
  allowed), if this parameter is off.
  3. Now if user enables include directive in postgresql.conf, it will
  enable Alter System as value of
  enable_alter_system is on.
  4. User can run Alter System command to disable Alter System
  enable_alter_system = off.
  Now even though include directive is enabled, but new Alter System
  commands will not work, however
  existing parameter's take into effect on restart/sighup.
 
 Yes.  Not sure that it'd be terribly likely for a user to do that, but
 if they do it, so be it.

With this design, if you put enable_alter_system=off in auto.conf, there
is no way for the user to enable alter system again short of editing a
file in the data directory.  I think this is one of the things that was
forbidden by policy; only files in the config directory needs to be
edited.

What I was proposing upthread is that enable_alter_system=off/on would
be present in postgresql.conf, and there is no include line for
auto.conf.  That way, if the user wishes to enable/disable the feature,
they need to edit postgresql.conf to do so.  ALTER SYSTEM doesn't offer
a way to disable itself.  If ALTER SYSTEM is disabled via
enable_alter_system=off in postgresql.conf, the settings in auto.conf
are not read.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Alvaro Herrera
Stephen Frost escribió:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
  Stephen Frost escribió:
   With includedir and include directives, and postgresql.conf setting a
   defined ordering, with last-wins, you could simply have the 'includedir'
   for conf.d come before the 'include' for auto.conf.
  
  Uh, I was thinking that conf.d would be read by the system
  automatically, not because of an includedir line in postgresql.conf.
 
 I'd much rather have an includedir directive than some hard-coded or
 command-line option to read the directory..  The directory should live
 in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives..

The conf.d/ path would be relative to postgresql.conf, so there's no
need for Debian to patch anything.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 What I was proposing upthread is that enable_alter_system=off/on would
 be present in postgresql.conf, and there is no include line for
 auto.conf.  That way, if the user wishes to enable/disable the feature,
 they need to edit postgresql.conf to do so.  ALTER SYSTEM doesn't offer
 a way to disable itself.  If ALTER SYSTEM is disabled via
 enable_alter_system=off in postgresql.conf, the settings in auto.conf
 are not read.

Personally, I'd get rid of any explicit enable_alter_system variable,
and instead have an include directive for auto.conf.  The functionality
you describe above is then available by commenting or uncommenting the
directive, plus the user has the option to decide where to put the
directive (and thus control which settings can or can't be overridden).
Anything else seems like it's going to be less flexible or else a lot
more complicated.

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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 With this design, if you put enable_alter_system=off in auto.conf, there
 is no way for the user to enable alter system again short of editing a
 file in the data directory.  I think this is one of the things that was
 forbidden by policy; only files in the config directory needs to be
 edited.

If you edit it by hand to begin with (setting that parameter to 'off')
then it's reasonable that you may have to edit it by hand again to fix
it.  If we don't want people to lock themselves out by using ALTER
SYSTEM to turn it off, then we just disallow that.

 What I was proposing upthread is that enable_alter_system=off/on would
 be present in postgresql.conf, and there is no include line for
 auto.conf.  

I really think that's a terrible approach, to be honest.  I want to see
an 'include' line in postgresql.conf for auto.conf, so the hapless
sysadmin who is trying to figure out what the crazy DBA did has some
clue what to look for.  enable_alter_system doesn't tell him diddly
about an 'auto.conf' file which is included in the system config.

 That way, if the user wishes to enable/disable the feature,
 they need to edit postgresql.conf to do so.  ALTER SYSTEM doesn't offer
 a way to disable itself.  

We can simply disallow ALTER SYSTEM from modifying enable_alter_system;
that strikes me as a reasonable thing to do anyway.  What I find a bit
more worrying is what happens if they decide to put
enable_alter_system=off into the postgresql.conf but keep the 'include'
line for auto.conf..  Which goes right back to the question that I had
before around if we want to complain when the same GUC is seen multiple
times during parsing.  It seems like there's no hope for it, given the
way this has been designed, because you *must* set certain parameters
and so you can't simply have them commented out, but those are likely to
be parameters which DBAs will want to change through ALTER SYSTEM.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost escribió:
  * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
   Stephen Frost escribió:
With includedir and include directives, and postgresql.conf setting a
defined ordering, with last-wins, you could simply have the 'includedir'
for conf.d come before the 'include' for auto.conf.
   
   Uh, I was thinking that conf.d would be read by the system
   automatically, not because of an includedir line in postgresql.conf.
  
  I'd much rather have an includedir directive than some hard-coded or
  command-line option to read the directory..  The directory should live
  in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives..
 
 The conf.d/ path would be relative to postgresql.conf, so there's no
 need for Debian to patch anything.

Uhhh, I really don't see that working, at all...

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 The conf.d/ path would be relative to postgresql.conf, so there's no
 need for Debian to patch anything.

 Uhhh, I really don't see that working, at all...

Why not?  conf.d is for installable files, AIUI.  What we need to
be writable is auto.conf, but I thought we'd agreed that would live
inside $PGDATA.

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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Stephen Frost sfr...@snowman.net writes:
  * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
  The conf.d/ path would be relative to postgresql.conf, so there's no
  need for Debian to patch anything.
 
  Uhhh, I really don't see that working, at all...
 
 Why not?  conf.d is for installable files, AIUI.  What we need to
 be writable is auto.conf, but I thought we'd agreed that would live
 inside $PGDATA.

I agree that auto.conf should live in $PGDATA, but I really don't like
the idea of conf.d being relative to some other existing file.  It
should be included through an 'includedir' option, not just picked up as
some magic directory name, and therefore consider the current
arrangement of parameters in Debian:

data_directory = '/var/lib/postgresql/9.2/main'
hba_file = '/etc/postgresql/9.2/main/pg_hba.conf'
ident_file = '/etc/postgresql/9.2/main/pg_ident.conf'

and postgres is started like so:

/usr/lib/postgresql/9.2/bin/postgres -D /var/lib/postgresql/9.2/main -c 
config_file=/etc/postgresql/9.2/main/postgresql.conf

With the proposed include line for auto.conf, which lives in $PGDATA,
we'd have:

include 'auto.conf'

Would we then have

includedir 'conf.d'

which is relative to postgresql.conf instead?

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Alvaro Herrera
Stephen Frost escribió:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
  Stephen Frost escribió:

   I'd much rather have an includedir directive than some hard-coded or
   command-line option to read the directory..  The directory should live
   in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives..
  
  The conf.d/ path would be relative to postgresql.conf, so there's no
  need for Debian to patch anything.
 
 Uhhh, I really don't see that working, at all...

I don't understand why not.  Care to explain?

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost escribió:
  * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
   Stephen Frost escribió:
 
I'd much rather have an includedir directive than some hard-coded or
command-line option to read the directory..  The directory should live
in /etc/postgresql/X.Y/cluster/ on at least Debian derivatives..
   
   The conf.d/ path would be relative to postgresql.conf, so there's no
   need for Debian to patch anything.
  
  Uhhh, I really don't see that working, at all...
 
 I don't understand why not.  Care to explain?

Tried to in my other mail, but let me also point out that we
(PGDG/Upstream) don't own the directory in which postgresql.conf
lives.  At least on Debian and relatives, that directory isn't under
$PGDATA and it already has other files in it beyond postgresql.conf or
even the other PostgreSQL config files of hba and ident.  Here's the
default directory setup on Debian for /etc/postgresql/9.2/main/:

-rw-r--r-- 1 postgres postgres   316 Jun 29 22:07 environment
-rw-r--r-- 1 postgres postgres   143 Jun 29 22:07 pg_ctl.conf
-rw-r- 1 postgres postgres  4649 Jun 29 22:07 pg_hba.conf
-rw-r- 1 postgres postgres  1636 Jun 29 22:07 pg_ident.conf
-rw-r--r-- 1 postgres postgres 19770 Jun 29 22:07 postgresql.conf
-rw-r--r-- 1 postgres postgres   378 Jun 29 22:07 start.conf

There's three other files there and some sysadmins may have already
created their own 'conf.d' directory, perhaps to use for building the
postgresql.conf file or similar.  We must have a way to disable the
conf.d option and a way to name it something other than 'conf.d', imv.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Alvaro Herrera
Stephen Frost escribió:

 I agree that auto.conf should live in $PGDATA, but I really don't like
 the idea of conf.d being relative to some other existing file.  It
 should be included through an 'includedir' option, not just picked up as
 some magic directory name, and therefore consider the current
 arrangement of parameters in Debian:
 
 data_directory = '/var/lib/postgresql/9.2/main'
 hba_file = '/etc/postgresql/9.2/main/pg_hba.conf'
 ident_file = '/etc/postgresql/9.2/main/pg_ident.conf'
 
 and postgres is started like so:
 
 /usr/lib/postgresql/9.2/bin/postgres -D /var/lib/postgresql/9.2/main -c 
 config_file=/etc/postgresql/9.2/main/postgresql.conf
 
 With the proposed include line for auto.conf, which lives in $PGDATA,
 we'd have:
 
 include 'auto.conf'
 
 Would we then have
 
 includedir 'conf.d'
 
 which is relative to postgresql.conf instead?

Well, all the relative paths in include/includedir directives would be
relative to the directory specified by the -c config_file param, which
makes perfect sense.  So the conf.d would work fine in your example.

The only problem I see in your snippet above is the include auto.conf
line, which doesn't make any sense because that file would not be found.
Hence my proposal that the file ought to be read automatically, not via
an include line.  Sadly I don't think we cannot just make it an absolute
path, in case the data dir is moved or whatever; the only choice I see
would be to have something like
include-data 'auto.conf'
or something like that which tells the system that the file is not in
the config dir but in the data dir instead.  A nearby comment could let
the user know about this file being in the data directory instead of the
config directory.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Alvaro Herrera
Stephen Frost escribió:

 Tried to in my other mail,

Yep, got it and replied, thanks.

 but let me also point out that we
 (PGDG/Upstream) don't own the directory in which postgresql.conf
 lives.  At least on Debian and relatives, that directory isn't under
 $PGDATA and it already has other files in it beyond postgresql.conf or
 even the other PostgreSQL config files of hba and ident.  Here's the
 default directory setup on Debian for /etc/postgresql/9.2/main/:
 
 -rw-r--r-- 1 postgres postgres   316 Jun 29 22:07 environment
 -rw-r--r-- 1 postgres postgres   143 Jun 29 22:07 pg_ctl.conf
 -rw-r- 1 postgres postgres  4649 Jun 29 22:07 pg_hba.conf
 -rw-r- 1 postgres postgres  1636 Jun 29 22:07 pg_ident.conf
 -rw-r--r-- 1 postgres postgres 19770 Jun 29 22:07 postgresql.conf
 -rw-r--r-- 1 postgres postgres   378 Jun 29 22:07 start.conf
 
 There's three other files there and some sysadmins may have already
 created their own 'conf.d' directory, perhaps to use for building the
 postgresql.conf file or similar.  We must have a way to disable the
 conf.d option and a way to name it something other than 'conf.d', imv.

Uhm.  I find it hard to care much for this position.  Surely config
files are not migrated automatically from one major version to the next,
so if somebody created a 9.3/main/conf.d directory, they will have to
change it when they migrate to 9.4.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Well, all the relative paths in include/includedir directives would be
 relative to the directory specified by the -c config_file param, which
 makes perfect sense.  So the conf.d would work fine in your example.

Why would include/includedir directives be relative to where the
'config_file' GUC is set to instead of relative to where all the other
GUCs in postgresql.conf are relative to?  That is a recipe for
confusion, imv.

Of course, the current situation is quite terrible anyway, imv.  Having
the GUCs be relative to whereever the user happens to run pg_ctl from is
pretty ugly- not to mention that the commented out 'defaults' won't
actually work if you uncomment them because the *actual* default/unset
value *is* in the data directory.  I'm starting to wish for a way to do
variable substitution inside postgresql.conf, so we could have defaults
that would actually work if uncommented (eg: $DataDir/pg_hba.conf).

That would help with auto.conf also.

 The only problem I see in your snippet above is the include auto.conf
 line, which doesn't make any sense because that file would not be found.

To be honest, I was considering 'include' to be relative to the data
directory and handled similar to how we process recovery.conf, but as we
consider paths in postgresql.conf to be relative to $PWD, that's not
ideal because it'd be different from the rest of the file references.

In any case, while the current situation sucks, I don't think we can go
changing how relative files are treated in postgresql.conf, nor should
we make the way a path is processed change depending on which GUC is
being set.

While I really like the 'include auto.conf' style, I'm starting to think
it may not be workable after all.  Another thing to consider is if the
user decides to change that include line..  What happens when the DBA
tries to do a 'ALTER SYSTEM'?  It'd still use the hard-coded auto.conf
file and happily update it, I imagine, but it won't actually get
included...

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Alvaro Herrera
Stephen Frost escribió:
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
  Well, all the relative paths in include/includedir directives would be
  relative to the directory specified by the -c config_file param, which
  makes perfect sense.  So the conf.d would work fine in your example.
 
 Why would include/includedir directives be relative to where the
 'config_file' GUC is set to instead of relative to where all the other
 GUCs in postgresql.conf are relative to?  That is a recipe for
 confusion, imv.
 
 Of course, the current situation is quite terrible anyway, imv.  Having
 the GUCs be relative to whereever the user happens to run pg_ctl from is
 pretty ugly- not to mention that the commented out 'defaults' won't
 actually work if you uncomment them because the *actual* default/unset
 value *is* in the data directory.

Uh?  See the docs:
http://www.postgresql.org/docs/devel/static/config-setting.html#CONFIG-INCLUDES

  ... the postgresql.conf file can contain include directives, ...
  If the file name is not an absolute path, it is taken as relative to
  the directory containing the referencing configuration file.


 I'm starting to wish for a way to do
 variable substitution inside postgresql.conf, so we could have defaults
 that would actually work if uncommented (eg: $DataDir/pg_hba.conf).
 
 That would help with auto.conf also.

Grumble.  I don't think we need this.  At least, not for ALTER SYSTEM or
conf.d.

  The only problem I see in your snippet above is the include auto.conf
  line, which doesn't make any sense because that file would not be found.
 
 To be honest, I was considering 'include' to be relative to the data
 directory and handled similar to how we process recovery.conf,

Well, recovery.conf is a special case that doesn't follow to normal
rules.

 but as we
 consider paths in postgresql.conf to be relative to $PWD, that's not
 ideal because it'd be different from the rest of the file references.

I don't know where you got that idea from, but it's wrong.

 While I really like the 'include auto.conf' style, I'm starting to think
 it may not be workable after all.  Another thing to consider is if the
 user decides to change that include line..  What happens when the DBA
 tries to do a 'ALTER SYSTEM'?  It'd still use the hard-coded auto.conf
 file and happily update it, I imagine, but it won't actually get
 included...

Well, this whole line of discussion started because I objected to the
whole code path that was trying to detect whether auto.conf had been
parsed, and raised a warning if ALTER SYSTEM was executed and the file
wasn't parsed.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Stephen Frost escribió:
 While I really like the 'include auto.conf' style, I'm starting to think
 it may not be workable after all.  Another thing to consider is if the
 user decides to change that include line..  What happens when the DBA
 tries to do a 'ALTER SYSTEM'?  It'd still use the hard-coded auto.conf
 file and happily update it, I imagine, but it won't actually get
 included...

 Well, this whole line of discussion started because I objected to the
 whole code path that was trying to detect whether auto.conf had been
 parsed, and raised a warning if ALTER SYSTEM was executed and the file
 wasn't parsed.

I really, really don't think that we should be trying to detect or prevent
any such thing.  If the user breaks it like that, he gets to keep both
pieces --- and who's to say it's broken, anyway?  Disabling ALTER SYSTEM
might have been exactly his intent.

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: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost escribió:
  Of course, the current situation is quite terrible anyway, imv.  Having
  the GUCs be relative to whereever the user happens to run pg_ctl from is
  pretty ugly- not to mention that the commented out 'defaults' won't
  actually work if you uncomment them because the *actual* default/unset
  value *is* in the data directory.
 
 Uh?  See the docs:
 http://www.postgresql.org/docs/devel/static/config-setting.html#CONFIG-INCLUDES
 
   ... the postgresql.conf file can contain include directives, ...
   If the file name is not an absolute path, it is taken as relative to
   the directory containing the referencing configuration file.

And what about

http://www.postgresql.org/docs/9.3/static/runtime-config-file-locations.html

 ... ?

   When setting any of these parameters, a relative path will be
   interpreted with respect to the directory in which postgres is
   started.

  I'm starting to wish for a way to do
  variable substitution inside postgresql.conf, so we could have defaults
  that would actually work if uncommented (eg: $DataDir/pg_hba.conf).
  
  That would help with auto.conf also.
 
 Grumble.  I don't think we need this.  At least, not for ALTER SYSTEM or
 conf.d.

imv, it would be handy.  Along with a $ConfDir which is the
postgresql.conf location- it would certainly make things clearer about
what files are being included from where.

  To be honest, I was considering 'include' to be relative to the data
  directory and handled similar to how we process recovery.conf,
 
 Well, recovery.conf is a special case that doesn't follow to normal
 rules.

I don't see why it should be.

  but as we
  consider paths in postgresql.conf to be relative to $PWD, that's not
  ideal because it'd be different from the rest of the file references.
 
 I don't know where you got that idea from, but it's wrong.

Not sure which you're referring to, but see above from the docs?  Also,
please tias..  I was amazed that we use $PWD also, but we do..

 Well, this whole line of discussion started because I objected to the
 whole code path that was trying to detect whether auto.conf had been
 parsed, and raised a warning if ALTER SYSTEM was executed and the file
 wasn't parsed.

I like the idea that we complain if ALTER SYSTEM ends up being
idempotent..  Not sure how far we should take that, but accepting
commands which end up doing nothing is bad, imv.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Alvaro Herrera
Stephen Frost escribió:

 And what about
 
 http://www.postgresql.org/docs/9.3/static/runtime-config-file-locations.html
 
  ... ?
 
When setting any of these parameters, a relative path will be
interpreted with respect to the directory in which postgres is
started.

That's talking about the data_directory and the various foo_file
settings, though; it doesn't apply to the include settings.  Note
especially that config_file says it can only be set on the postgres
command line.  (I think a saner definition would have been to state that
relative paths are not allowed in the command line.  But that ship has
already sailed.  And relative paths seem useful in the config file; and
maintaining the distinction that they are allowed within the config file
but not in the command line might be awkward.)


(Uhm, when a command line contains stuff, is the stuff in the command
line or on it?)

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Stephen Frost
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost escribió:
  http://www.postgresql.org/docs/9.3/static/runtime-config-file-locations.html
 
 That's talking about the data_directory and the various foo_file
 settings, though; it doesn't apply to the include settings.  

Right- that's what I'm bitching about.  We have various references to
file locations, with defined handling of relative locations, and the
'include' system completely ignores that and instead invents its own
making the result a confusing mess.

 Note
 especially that config_file says it can only be set on the postgres
 command line.  (I think a saner definition would have been to state that
 relative paths are not allowed in the command line.  But that ship has
 already sailed.  And relative paths seem useful in the config file; and
 maintaining the distinction that they are allowed within the config file
 but not in the command line might be awkward.)

Relative paths based on $PWD are useful?  Really?  Not on my systems
anyway..

 (Uhm, when a command line contains stuff, is the stuff in the command
 line or on it?)

A just question- I vote 'on'. :)

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Martijn van Oosterhout
On Tue, Aug 20, 2013 at 11:23:06AM -0400, Stephen Frost wrote:
  What I was proposing upthread is that enable_alter_system=off/on would
  be present in postgresql.conf, and there is no include line for
  auto.conf.  
 
 I really think that's a terrible approach, to be honest.  I want to see
 an 'include' line in postgresql.conf for auto.conf, so the hapless
 sysadmin who is trying to figure out what the crazy DBA did has some
 clue what to look for.  enable_alter_system doesn't tell him diddly
 about an 'auto.conf' file which is included in the system config.

ISTM you want some kind of hybrid setting like:

#include_system auto.conf

which simultaneously does three things:

1. Sets the enable_alter_system flag
2. Indicates the file to use
3. Indicates the priority of the setting re other settings.

Comment it out, ALTER SYSTEM stop working. Put it back and it's
immediately clear what it means. And the user can control where the
settings go.

Syntax is a bit fugly though.

Have a nice day,
-- 
Martijn van Oosterhout   klep...@svana.org   http://svana.org/kleptog/
 He who writes carelessly confesses thereby at the very outset that he does
 not attach much importance to his own thoughts.
   -- Arthur Schopenhauer


signature.asc
Description: Digital signature


Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL [review])

2013-08-20 Thread Bruce Momjian
On Tue, Aug 20, 2013 at 08:38:52AM +0530, Amit Kapila wrote:
 On Tue, Aug 20, 2013 at 8:27 AM, Stephen Frost sfr...@snowman.net wrote:
  * Amit Kapila (amit.kapil...@gmail.com) wrote:
  If both are okay, then I would like to go with second option (include
  file mechanism).
  I think by default, we should allow auto file to be included and if
  user faces any problem or otherwise,
  then he can decide to disable it.
 
  If it's enabled by default, then we need to ship an 'auto' file which is
  empty by default...
 
 initdb will create the empty auto file. If we don't enable by default,
 then if user uses
 ALTER SYSTEM and do sighup/restart, changed config parameter values
 will not come into affect
 until he manually enables it by removing '#' from '#include'.

Just a heads-up, but a lot of C language folks are going to look at:

#include abc

and think that is enabled.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


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


  1   2   3   4   >