Re: [HACKERS] includeifexists in configuration file
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
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
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
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
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
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
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
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
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
[HACKERS] includeifexists in configuration file
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
para
indexterm
+ primaryliteralincludeifexists//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
+ literalincludeifexists/ does not.
+/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..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)
{
/*
