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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

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(at)timbira(dot)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(at)2ndquadrant(dot)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(at)2ndQuadrant(dot)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(at)rice(dot)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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

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

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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us