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 
. 
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-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 
. 
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/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 
  
 
  
+  include_if_exists
+  in configuration file
+ 
+ Use the same approach as the include directive, continuing
+ normally if the file does not exist.  A regular include
+ will stop with an error if the referenced file is missing, while
+ include_if_exists does not.  A warning about the missing
+ file will be logged.
+
+ 
+
+ 
   SIGHUP
  
  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,
!  depth + 1, elevel,
!  head_p, tail_p))
! OK = false;
! 			yy_switch_to_buffer(lex_buffer

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-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 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-11-16 Thread Robert Haas 
On Wed, Nov 16, 2011 at 12:28 AM, 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 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

Re: [HACKERS] includeifexists in configuration file

2011-11-16 Thread Tom Lane 
Euler Taveira de Oliveira  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 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

[HACKERS] includeifexists in configuration file

2011-11-15 Thread Greg Smith 
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.  Demo:


$ tail -n 1 $PGDATA/postgresql.conf
include 'missing.conf'
$ pg_ctl start -l $PGLOG
server starting
$ tail -n 2 $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

$ vi $PGDATA/postgresql.conf
$ tail -n 1 $PGDATA/postgresql.conf
includeifexists 'missing.conf'
$ pg_ctl start -l $PGLOG
server starting
$ tail -n 3 $PGLOG
LOG:  database system was shut down at 2011-11-16 00:17:36 EST
LOG:  database system is ready to accept connections
LOG:  autovacuum launcher started

There might be a cleaner way to write this that eliminates some of the 
cut and paste duplication between this and the regular include 
directive.  I'm short on clever but full of brute force tonight.


--
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..da45ac1 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*** include 'filename'
*** 91,96 
--- 91,107 
  
 
  
+  includeifexists
+  in configuration file
+ 
+ Use the same approach as the include directive, continuing
+ normally if the file does not exist.  A regular include
+ will stop with an error if the referenced file is missing, while
+ includeifexists does not.
+
+ 
+
+ 
   SIGHUP
  
  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..6f26421 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;
! 		}
! 
! 		/* Silently skip missing files if not asked to be strict */
! 		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, "includeifexists") == 0)
! 		{
! 			/*
! 			 * An includeifexists directive isn't a variable and should be
! 			 * processed immediately.
! 			 */
! 			unsigned int save_ConfigFileLineno = ConfigFileLineno;
! 
! 			if (!ParseConfigFile(opt_value, config_file, false,
!  depth + 1, elevel,
!  head_p, tail_p))
! OK = false;
! 			yy_switch_to_buffer(lex_buffer);
! 			ConfigFileLineno = save_ConfigFileLineno;
! 			pfree(opt_name);
! 			pfree(opt_value);
! 		}
! 		else if (guc_name_compare(opt_name, "include") == 0)
  		{
  			/*
  			 * An include directive isn't a variable and should be processed
*** ParseConfigFp(FILE *fp, c