Re: [HACKERS] includeifexists in configuration file

2011-12-15 Thread Greg Smith

On 12/12/2011 04:47 PM, Andrew Dunstan wrote:
I have briefly looked at the code (but not tried to apply or build 
it), and modulo the naming issue it looks OK to me.
Unless there is some other issue let's just get it applied. It looks 
like almost a no-brainer to me.


It isn't very fancy, but is does something people that can fit into a 
couple of use-cases.  Attached update has two changes to address the 
suggestions I got, which closes everything I knew about with this one:


-It's now include_if_exists
-Files that are skipped are logged now

So current behavior:

$ tail -n 1 postgresql.conf
include 'missing.conf'
$ start
server starting
$ tail $PGLOG
LOG:  could not open configuration file 
/home/gsmith/pgwork/data/include-exists/missing.conf: No such file or 
directory
FATAL:  configuration file 
/home/gsmith/pgwork/data/include-exists/postgresql.conf contains errors


And new behavior:

$ vi $PGDATA/postgresql.conf
$ tail -n 1 postgresql.conf
include_if_exists 'missing.conf'
$ start
server starting
$ tail $PGLOG
LOG:  skipping missing configuration file 
/home/gsmith/pgwork/data/include-exists/missing.conf

LOG:  database system was shut down at 2011-12-15 06:48:46 EST
LOG:  database system is ready to accept connections

--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index d1e628f..0cc3296 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** include 'filename'
*** 91,96 
--- 91,108 
  
 para
  indexterm
+  primaryliteralinclude_if_exists//primary
+  secondaryin configuration file/secondary
+ /indexterm
+ Use the same approach as the literalinclude/ directive, continuing
+ normally if the file does not exist.  A regular literalinclude/
+ will stop with an error if the referenced file is missing, while
+ literalinclude_if_exists/ does not.  A warning about the missing
+ file will be logged.
+/para
+ 
+para
+ indexterm
   primarySIGHUP/primary
  /indexterm
  The configuration file is reread whenever the main server process receives a
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index a094c7a..6ba130c 100644
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
*** ProcessConfigFile(GucContext context)
*** 129,135 
  	/* Parse the file into a list of option names and values */
  	head = tail = NULL;
  
! 	if (!ParseConfigFile(ConfigFileName, NULL, 0, elevel, head, tail))
  	{
  		/* Syntax error(s) detected in the file, so bail out */
  		error = true;
--- 129,135 
  	/* Parse the file into a list of option names and values */
  	head = tail = NULL;
  
! 	if (!ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, head, tail))
  	{
  		/* Syntax error(s) detected in the file, so bail out */
  		error = true;
*** ProcessConfigFile(GucContext context)
*** 363,369 
   * and absolute-ifying the path name if necessary.
   */
  bool
! ParseConfigFile(const char *config_file, const char *calling_file,
  int depth, int elevel,
  ConfigVariable **head_p,
  ConfigVariable **tail_p)
--- 363,369 
   * and absolute-ifying the path name if necessary.
   */
  bool
! ParseConfigFile(const char *config_file, const char *calling_file, bool strict,
  int depth, int elevel,
  ConfigVariable **head_p,
  ConfigVariable **tail_p)
*** ParseConfigFile(const char *config_file,
*** 414,424 
  	fp = AllocateFile(config_file, r);
  	if (!fp)
  	{
! 		ereport(elevel,
! (errcode_for_file_access(),
!  errmsg(could not open configuration file \%s\: %m,
! 		config_file)));
! 		return false;
  	}
  
  	OK = ParseConfigFp(fp, config_file, depth, elevel, head_p, tail_p);
--- 414,430 
  	fp = AllocateFile(config_file, r);
  	if (!fp)
  	{
! 		if (strict)
! 		{
! 			ereport(elevel,
! 	(errcode_for_file_access(),
! 	 errmsg(could not open configuration file \%s\: %m,
! 			config_file)));
! 			return false;
! 		}
! 
! 		elog(LOG, skipping missing configuration file \%s\, config_file);
! 		return OK;
  	}
  
  	OK = ParseConfigFp(fp, config_file, depth, elevel, head_p, tail_p);
*** ParseConfigFp(FILE *fp, const char *conf
*** 512,518 
  		}
  
  		/* OK, process the option name and value */
! 		if (guc_name_compare(opt_name, include) == 0)
  		{
  			/*
  			 * An include directive isn't a variable and should be processed
--- 518,541 
  		}
  
  		/* OK, process the option name and value */
! 		if (guc_name_compare(opt_name, include_if_exists) == 0)
! 		{
! 			/*
! 			 * An include_if_exists directive isn't a variable and should be
! 			 * processed immediately.
! 			 */
! 			unsigned int save_ConfigFileLineno = ConfigFileLineno;
! 
! 			if (!ParseConfigFile(opt_value, config_file, false,
! 			

Re: [HACKERS] includeifexists in configuration file

2011-12-15 Thread Andrew Dunstan



On 12/15/2011 06:54 AM, Greg Smith wrote:

On 12/12/2011 04:47 PM, Andrew Dunstan wrote:
I have briefly looked at the code (but not tried to apply or build 
it), and modulo the naming issue it looks OK to me.
Unless there is some other issue let's just get it applied. It looks 
like almost a no-brainer to me.


It isn't very fancy, but is does something people that can fit into a 
couple of use-cases.  Attached update has two changes to address the 
suggestions I got, which closes everything I knew about with this one:


-It's now include_if_exists
-Files that are skipped are logged now

So current behavior:

$ tail -n 1 postgresql.conf
include 'missing.conf'
$ start
server starting
$ tail $PGLOG
LOG:  could not open configuration file 
/home/gsmith/pgwork/data/include-exists/missing.conf: No such file 
or directory
FATAL:  configuration file 
/home/gsmith/pgwork/data/include-exists/postgresql.conf contains errors


And new behavior:

$ vi $PGDATA/postgresql.conf
$ tail -n 1 postgresql.conf
include_if_exists 'missing.conf'
$ start
server starting
$ tail $PGLOG
LOG:  skipping missing configuration file 
/home/gsmith/pgwork/data/include-exists/missing.conf

LOG:  database system was shut down at 2011-12-15 06:48:46 EST
LOG:  database system is ready to accept connections


Committed. I changed the elog() call to use ereport(): you're not 
supposed to use elog() for things we expect might well happen and cause 
log entries - see bottom of 
http://www.postgresql.org/docs/current/static/error-message-reporting.html. 
I've probably been guilty of this in the past, it's a bit too easy to 
forget.


cheers

andrew

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


Re: [HACKERS] includeifexists in configuration file

2011-12-15 Thread Greg Smith

On 12/15/2011 08:16 PM, Andrew Dunstan wrote:
 I changed the elog() call to use ereport(): you're not supposed to 
use elog() for things we expect might well happen and cause log 
entries - see bottom of 
http://www.postgresql.org/docs/current/static/error-message-reporting.html. 
I've probably been guilty of this in the past, it's a bit too easy to 
forget.


Quite, I both knew this once and forgot it last night.  There was some 
nagging in the back of my head that I was doing something wrong, but I 
couldn't place what.  Happy this is committed, given that I've suggested 
relying upon it in the recovery.conf thread.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


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


Re: [HACKERS] includeifexists in configuration file

2011-12-12 Thread Greg Smith

On 11/16/2011 10:19 AM, Robert Haas wrote:

I haven't read the code yet, but just to get the bikeshedding started,
I think it might be better to call this include_if_exists rather than
running it together as one word.
   


What's going on, it's like this bikeshed just disappeared.  I should 
figure out how that happened so I can replicate it.


This naming style change sounds fine to me, and I just adopted it for 
the updated configuration directory patch.  That patch now rearranges 
the documentation this feature modifies.  This is a pretty trivial 
feature I'm not real concerned about getting a review for.  I'll update 
this with the name change and appropriate rebased patch once some 
decision has been made about that one; will just bounce this forward to 
January if it's still here when the current CF starts closing in earnest.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


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


Re: [HACKERS] includeifexists in configuration file

2011-12-12 Thread Ross Reedstrom
On Mon, Dec 12, 2011 at 02:24:53PM -0500, Greg Smith wrote:
 On 11/16/2011 10:19 AM, Robert Haas wrote:
 I haven't read the code yet, but just to get the bikeshedding started,
 I think it might be better to call this include_if_exists rather than
 running it together as one word.
 
 What's going on, it's like this bikeshed just disappeared.  I should
 figure out how that happened so I can replicate it.

Must be that special camo paint.


Ross
Woohoo! Caught up from my beginning of Oct. trip backlog, just in time for 
Christmas!
-- 
Ross Reedstrom, Ph.D. reeds...@rice.edu
Systems Engineer  Admin, Research Scientistphone: 713-348-6166
Connexions  http://cnx.orgfax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E  F888 D3AE 810E 88F0 BEDE

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


Re: [HACKERS] includeifexists in configuration file

2011-12-12 Thread Andrew Dunstan



On 12/12/2011 02:24 PM, Greg Smith wrote:

On 11/16/2011 10:19 AM, Robert Haas wrote:

I haven't read the code yet, but just to get the bikeshedding started,
I think it might be better to call this include_if_exists rather than
running it together as one word.


What's going on, it's like this bikeshed just disappeared.  I should 
figure out how that happened so I can replicate it.


This naming style change sounds fine to me, and I just adopted it for 
the updated configuration directory patch.  That patch now rearranges 
the documentation this feature modifies.  This is a pretty trivial 
feature I'm not real concerned about getting a review for.  I'll 
update this with the name change and appropriate rebased patch once 
some decision has been made about that one; will just bounce this 
forward to January if it's still here when the current CF starts 
closing in earnest.





I have briefly looked at the code (but not tried to apply or build it), 
and modulo the naming issue it looks OK to me.


Unless there is some other issue let's just get it applied. It looks 
like almost a no-brainer to me.


cheers

andrew

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


Re: [HACKERS] includeifexists in configuration file

2011-11-16 Thread Euler Taveira de Oliveira
On 16-11-2011 02:28, Greg Smith wrote:
 By recent popular request in the ongoing discussion saga around merging the
 recovery.conf, I've added an includeifexists directive to the
 postgresql.conf in the attached patch.
 
I'm not following the merging recovery.conf thread but isn't it worth emitting
at least an WARNING message when the file does not exist?

Something like

WARNING:  could not open configuration file /foo/missing.conf, skipping

Let's suppose a DBA is using this new feature to include some general company
recommendations. If (s)he mistyped the name of the file, the general
recommendations will not be applied and the DBA won't be even warned. That's
not what a DBA would expect.


-- 
   Euler Taveira de Oliveira - Timbira   http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

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


Re: [HACKERS] includeifexists in configuration file

2011-11-16 Thread Tom Lane
Euler Taveira de Oliveira eu...@timbira.com writes:
 On 16-11-2011 02:28, Greg Smith wrote:
 By recent popular request in the ongoing discussion saga around merging the
 recovery.conf, I've added an includeifexists directive to the
 postgresql.conf in the attached patch.

 I'm not following the merging recovery.conf thread but isn't it worth emitting
 at least an WARNING message when the file does not exist?

 Something like

 WARNING:  could not open configuration file /foo/missing.conf, skipping

Minor note here: people keep thinking that WARNING  LOG with respect to
messages that can only go to the server log.  This is not correct ...
LOG would be the right elevel to use.

regards, tom lane

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


Re: [HACKERS] includeifexists in configuration file

2011-11-16 Thread Robert Haas
On Wed, Nov 16, 2011 at 12:28 AM, Greg Smith g...@2ndquadrant.com wrote:
 By recent popular request in the ongoing discussion saga around merging the
 recovery.conf, I've added an includeifexists directive to the
 postgresql.conf in the attached patch.

I haven't read the code yet, but just to get the bikeshedding started,
I think it might be better to call this include_if_exists rather than
running it together as one word.

-- 
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