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 US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
Attachments:
include-exists-v1.patchtext/x-patch; name=include-exists-v1.patchDownload
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>
+ <primary><literal>includeifexists</></primary>
+ <secondary>in configuration file</secondary>
+ </indexterm>
+ Use the same approach as the <literal>include</> directive, continuing
+ normally if the file does not exist. A regular <literal>include</>
+ will stop with an error if the referenced file is missing, while
+ <literal>includeifexists</> does not.
+ </para>
+
+ <para>
+ <indexterm>
<primary>SIGHUP</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)
{
/*
* An include directive isn't a variable and should be processed
*************** ParseConfigFp(FILE *fp, const char *conf
*** 520,526 ****
*/
unsigned int save_ConfigFileLineno = ConfigFileLineno;
! if (!ParseConfigFile(opt_value, config_file,
depth + 1, elevel,
head_p, tail_p))
OK = false;
--- 543,549 ----
*/
unsigned int save_ConfigFileLineno = ConfigFileLineno;
! if (!ParseConfigFile(opt_value, config_file, true,
depth + 1, elevel,
head_p, tail_p))
OK = false;
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 8e3057a..52109e5 100644
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*************** typedef struct ConfigVariable
*** 111,117 ****
} ConfigVariable;
extern bool ParseConfigFile(const char *config_file, const char *calling_file,
! int depth, int elevel,
ConfigVariable **head_p, ConfigVariable **tail_p);
extern bool ParseConfigFp(FILE *fp, const char *config_file,
int depth, int elevel,
--- 111,117 ----
} ConfigVariable;
extern bool ParseConfigFile(const char *config_file, const char *calling_file,
! bool strict, int depth, int elevel,
ConfigVariable **head_p, ConfigVariable **tail_p);
extern bool ParseConfigFp(FILE *fp, const char *config_file,
int depth, int elevel,
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
Euler Taveira de Oliveira <euler@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
On Wed, Nov 16, 2011 at 12:28 AM, Greg Smith <greg@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
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 US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
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. reedstrm@rice.edu
Systems Engineer & Admin, Research Scientist phone: 713-348-6166
Connexions http://cnx.org fax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE
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
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 US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
Attachments:
include-exists-v2.patchtext/x-patch; name=include-exists-v2.patchDownload
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>
+ <primary><literal>include_if_exists</></primary>
+ <secondary>in configuration file</secondary>
+ </indexterm>
+ Use the same approach as the <literal>include</> directive, continuing
+ normally if the file does not exist. A regular <literal>include</>
+ will stop with an error if the referenced file is missing, while
+ <literal>include_if_exists</> does not. A warning about the missing
+ file will be logged.
+ </para>
+
+ <para>
+ <indexterm>
<primary>SIGHUP</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,
! 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, const char *conf
*** 520,526 ****
*/
unsigned int save_ConfigFileLineno = ConfigFileLineno;
! if (!ParseConfigFile(opt_value, config_file,
depth + 1, elevel,
head_p, tail_p))
OK = false;
--- 543,549 ----
*/
unsigned int save_ConfigFileLineno = ConfigFileLineno;
! if (!ParseConfigFile(opt_value, config_file, true,
depth + 1, elevel,
head_p, tail_p))
OK = false;
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 8e3057a..52109e5 100644
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*************** typedef struct ConfigVariable
*** 111,117 ****
} ConfigVariable;
extern bool ParseConfigFile(const char *config_file, const char *calling_file,
! int depth, int elevel,
ConfigVariable **head_p, ConfigVariable **tail_p);
extern bool ParseConfigFp(FILE *fp, const char *config_file,
int depth, int elevel,
--- 111,117 ----
} ConfigVariable;
extern bool ParseConfigFile(const char *config_file, const char *calling_file,
! bool strict, int depth, int elevel,
ConfigVariable **head_p, ConfigVariable **tail_p);
extern bool ParseConfigFp(FILE *fp, const char *config_file,
int depth, int elevel,
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 nowSo 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 errorsAnd 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
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 US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us