Thread

1. Configuration include directory

   Greg Smith <greg@2ndquadrant.com> — 2011-11-16T04:53:53Z

   Two years ago Magnus submitted a patch to parse all the configuration 
   files in a directory.  After some discussion I tried to summarize what I 
   thought the most popular ideas were for moving that forward:
   
   http://archives.postgresql.org/pgsql-hackers/2009-10/msg01452.php
   http://archives.postgresql.org/pgsql-hackers/2009-10/msg01631.php
   
   And I've now cleared the bit rot and updated that patch to do what was 
   discussed.  Main feature set:
   
   -Called by specifying "includedir <directory>".  No changes to the 
   shipped postgresql.conf yet.
   -Takes an input directory name
   -If it's not an absolute path, considers that relative to the -D option 
   (if specified) or PGDATA, the same logic used to locate the 
   postgresql.conf (unless a full path to it is used)
   -Considers all names in that directory that end with *.conf  [Discussion 
   concluded more flexibility here would be of limited value relative to 
   how it complicates the implementation]
   -Loops over the files found in sorted order by name
   
   The idea here is that it will be easier to write tools that customize 
   the database configuration if they can just write a new file out, rather 
   than needing to parse the whole configuration file first.  This would 
   allow Apache style configuration directories.  My end goal here is to 
   see all of the work initdb does pushed into a new file included by this 
   scheme.  People could then expect a functionally empty postgresql.conf 
   except for an includedir, and the customization would go into 00initdb.  
   Getting some agreement on that is not necessary for this feature to go 
   in though; one step at a time.
   
   Here's an example showing this working, including rejection of a 
   spurious editor backup file in the directory:
   
   $ cat $PGDATA/postgresql.conf | grep ^work_mem
   $ tail -n 1 $PGDATA/postgresql.conf
   includedir='conf.d'
   $ ls $PGDATA/conf.d
   00config.conf  00config.conf~
   $ cat $PGDATA/conf.d/00config.conf
   work_mem=4MB
   $ cat $PGDATA/conf.d/00config.conf~
   work_mem=2MB
   $ psql -c "select name,setting,sourcefile,sourceline from pg_settings 
   where name='work_mem'"
       name   | setting |                      
   sourcefile                       | sourceline
   ----------+---------+-------------------------------------------------------+------------
     work_mem | 4096    | 
   /home/gsmith/pgwork/data/confdir/conf.d/00config.conf |          1
   
   No docs in here yet.  There's one ugly bit of code here I was hoping 
   (but failed) to avoid.  Right now the server doesn't actually save the 
   configuration directory anywhere.  Once you leave the initial read in 
   SelectConfigFiles, that information is gone, and you only have the 
   configfile.  I decided to make that configdir into a global value.  
   Seemed easier than trying to pass it around, given how many SIGHUP paths 
   could lead to this new code.
   
   I can see some potential confusion here in one case.  Let's say someone 
   specifies a full path to their postgresql.conf file.  They might assume 
   that the includedir was relative to the directory that file is in.  
   Let's say configfile is /etc/sysconfig/pgsql/postgresql.conf ; a user 
   might think that "includedir conf.d" from there would reference 
   /etc/sysconfig/pgsql/conf.d instead of the $PGDATA/conf.d you actually 
   get.  Wavering on how to handle that is one reason I didn't try 
   documenting this yet, the decision I made here may not actually be the 
   right one.
   
   -- 
   Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
   PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
   
   

2. Re: Configuration include directory

   Greg Jaskiewicz <gj@pointblue.com.pl> — 2011-11-16T23:45:24Z

   On 16 Nov 2011, at 04:53, Greg Smith wrote:
   > 
   > -Called by specifying "includedir <directory>".  No changes to the shipped postgresql.conf yet.
   > -Takes an input directory name
   Very useful idea. 
   
   What will happen if I specify:
   
   includedir './'
   
   Ie, what about potential cyclic dependency. 
   
   
   

3. Re: Configuration include directory

   Andrew Dunstan <andrew@dunslane.net> — 2011-11-17T00:18:03Z

   On Wed, November 16, 2011 6:45 pm, Greg Jaskiewicz wrote:
   >
   > On 16 Nov 2011, at 04:53, Greg Smith wrote:
   >>
   >> -Called by specifying "includedir <directory>".  No changes to the
   >> shipped postgresql.conf yet.
   >> -Takes an input directory name
   > Very useful idea.
   >
   > What will happen if I specify:
   >
   > includedir './'
   >
   > Ie, what about potential cyclic dependency.
   >
   
   
   I would vote for it only to handle plain files (possibly softlinked) in
   the named directory.
   
   cheers
   
   andrew
   
   
   

4. Re: Configuration include directory

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-17T01:52:35Z

   "Andrew Dunstan" <andrew@dunslane.net> writes:
   > On Wed, November 16, 2011 6:45 pm, Greg Jaskiewicz wrote:
   >> What will happen if I specify:
   >> includedir './'
   
   > I would vote for it only to handle plain files (possibly softlinked) in
   > the named directory.
   
   I think Greg's point is that that would lead to again reading
   postgresql.conf, and then again processing the includedir directive,
   lather rinse repeat till stack overflow.
   
   Now one view of this is that we already expect postgresql.conf to only
   be writable by responsible adults, so if a DBA breaks his database this
   way he has nobody but himself to blame.  But still, if there's a simple
   way to define that risk away, it wouldn't be a bad thing.
   
   (Do we guard against recursive inclusion via plain old include?  If
   not, maybe this isn't worth worrying about.)
   
   			regards, tom lane
   
   

5. Re: Configuration include directory

   Alvaro Herrera <alvherre@commandprompt.com> — 2011-11-17T13:34:48Z

   Excerpts from Tom Lane's message of mié nov 16 22:52:35 -0300 2011:
   
   > (Do we guard against recursive inclusion via plain old include?  If
   > not, maybe this isn't worth worrying about.)
   
   Yes, we do
   
   FATAL:  could not open configuration file "foo.conf": maximum nesting depth exceeded
   
   -- 
   Álvaro Herrera <alvherre@commandprompt.com>
   The PostgreSQL Company - Command Prompt, Inc.
   PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   
   

6. Re: Configuration include directory

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-17T16:03:45Z

   Alvaro Herrera <alvherre@commandprompt.com> writes:
   > Excerpts from Tom Lane's message of mi nov 16 22:52:35 -0300 2011:
   >> (Do we guard against recursive inclusion via plain old include?  If
   >> not, maybe this isn't worth worrying about.)
   
   > Yes, we do
   
   > FATAL:  could not open configuration file "foo.conf": maximum nesting depth exceeded
   
   Oh, right.  So as long as the include-directory code path doesn't
   interfere with tracking that nesting depth, I don't think it needs
   any extra protection against include-the-same-directory.
   
   			regards, tom lane
   
   

7. Re: Configuration include directory

   Noah Misch <noah@leadboat.com> — 2011-12-06T06:16:48Z

   Hi Greg,
   
   On Tue, Nov 15, 2011 at 11:53:53PM -0500, Greg Smith wrote:
   > Two years ago Magnus submitted a patch to parse all the configuration  
   > files in a directory.  After some discussion I tried to summarize what I  
   > thought the most popular ideas were for moving that forward:
   >
   > http://archives.postgresql.org/pgsql-hackers/2009-10/msg01452.php
   > http://archives.postgresql.org/pgsql-hackers/2009-10/msg01631.php
   
   What a thread.  I think the earlier patch was more controversial due to its
   introduction of policy, a single include directory searched by default.  This
   latest patch is just infrastructure through which individual sites can build
   all manner of configuration strategies.  Many projects implement similar
   directives for their configuration files, so I'm quite comfortable assuming
   that some sites/packagers will like this.
   
   > -If it's not an absolute path, considers that relative to the -D option  
   > (if specified) or PGDATA, the same logic used to locate the  
   > postgresql.conf (unless a full path to it is used)
   
   Let's instead mimic the behavior of the "include" directive, which finds its
   target relative to the file containing the directive.  This also removes the
   warts you mentioned in your final two paragraphs:
   
   > No docs in here yet.  There's one ugly bit of code here I was hoping  
   > (but failed) to avoid.  Right now the server doesn't actually save the  
   > configuration directory anywhere.  Once you leave the initial read in  
   > SelectConfigFiles, that information is gone, and you only have the  
   > configfile.  I decided to make that configdir into a global value.   
   > Seemed easier than trying to pass it around, given how many SIGHUP paths  
   > could lead to this new code.
   
   SelectConfigFiles() still does "free(configdir)".  Due to that, in my testing,
   SIGHUP reloads do not re-find relative includedirs.
   
   > I can see some potential confusion here in one case.  Let's say someone  
   > specifies a full path to their postgresql.conf file.  They might assume  
   > that the includedir was relative to the directory that file is in.   
   > Let's say configfile is /etc/sysconfig/pgsql/postgresql.conf ; a user  
   > might think that "includedir conf.d" from there would reference  
   > /etc/sysconfig/pgsql/conf.d instead of the $PGDATA/conf.d you actually  
   > get.  Wavering on how to handle that is one reason I didn't try  
   > documenting this yet, the decision I made here may not actually be the  
   > right one.
   
   For this patch, the documentation is perhaps more important than the code.
   
   > *** a/src/backend/utils/misc/guc-file.l
   > --- b/src/backend/utils/misc/guc-file.l
   
   > *************** ParseConfigFp(FILE *fp, const char *conf
   > *** 599,604 ****
   > --- 616,727 ----
   >   	return OK;
   >   }
   >   
   > + static int
   > + comparestr(const void *a, const void *b)
   > + {
   > + 	return strcmp(*(char **) a, *(char **) b);
   > + }
   > + 
   > + /*
   > +  * Read and parse all config files in a subdirectory in alphabetical order
   > +  */
   > + bool
   > + ParseConfigDirectory(const char *includedir,
   > + 				const char *calling_file,
   > + 				int depth, int elevel,
   > + 				ConfigVariable **head_p,
   > + 				ConfigVariable **tail_p)
   > + {
   > + 	DIR *d;
   > + 	struct dirent *de;
   > + 	char directory[MAXPGPATH];
   > + 	char **filenames = NULL;
   > + 	int num_filenames = 0;
   > + 	int size_filenames = 0;
   > + 	bool status;
   > + 
   > + 	if (is_absolute_path(includedir))
   > + 		sprintf(directory, "%s", includedir);
   > + 	else
   > + 		sprintf(directory, "%s/%s", configdir, includedir);
   
   You need a length-limiting copy, and this won't cut it on Windows.  I suggest
   extracting and reusing the comparable logic in ParseConfigFile().
   
   > + 	d = AllocateDir(directory);
   > + 	if (d == NULL)
   > + 	{
   > + 		/*
   > + 		 * Not finding the configuration directory is not fatal, because we
   > + 		 * still have the main postgresql.conf file. Return true so the
   > + 		 * complete config parsing doesn't fail in this case. Also avoid
   > + 		 * logging this, since it can be a normal situtation.
   > + 		 */
   > + 		return true;
   
   I can't see much to recommend this; it's morally equivalent to silently
   ignoring "include somefile" or "work_mem = 'foobar'".
   
   > + 	}
   > + 
   > + 	/*
   > + 	 * Read the directory and put the filenames in an array, so we can sort
   > + 	 * them prior to processing the contents.
   > + 	 */
   > + 	while ((de = ReadDir(d, directory)) != NULL)
   > + 	{
   > + 		struct stat st;
   > + 		char filename[MAXPGPATH];
   > + 
   > + 		/*
   > + 		 * Only parse files with names ending in ".conf".
   > + 		 * This automatically excludes things like "." and "..", as well
   > + 		 * as backup files and editor debris.
   > + 		 */
   > + 		if (strlen(de->d_name) < 6)
   > + 			continue;
   
   I would probably allow the literal file name ".conf" as well, mainly to avoid
   documenting the need for a nonempty prefix.
   
   > + 		if (strcmp(de->d_name + strlen(de->d_name) - 5, ".conf") != 0)
   > + 			continue;
   
   That may as well be memcmp().
   
   > + 
   > + 		snprintf(filename, MAXPGPATH, "%s/%s", directory, de->d_name);
   
   If snprintf() exhausts the buffer, you're left with an unterminated string. 
   
   > + 		if (stat(filename, &st) == 0)
   
   We could perhaps proceed silently after ENOENT, but any other error from
   stat() should get reported.  Since ENOENT should only happen when the file
   gets deleted during the run of this function, I'd just go ahead and make any
   stat failure produce an error.
   
   > + 		{
   > + 			if (!S_ISDIR(st.st_mode))
   > + 			{
   > + 				/* Add file to list */
   > + 				if (num_filenames == size_filenames)
   > + 				{
   > + 					/* Increase size of array in blocks of 32 */
   > + 					size_filenames += 32;
   > + 					filenames = guc_realloc(elevel, filenames, size_filenames * sizeof(char *));
   
   While our usage isn't 100% consistent, I think guc_realloc() is mainly for
   permanent-lifetime objects.  repalloc() is fine for this transient use.  You
   also need not export guc_realloc(), then.
   
   > + 				}
   > + 				filenames[num_filenames] = strdup(filename);
   
   Use pstrdup().
   
   > + 				num_filenames++;
   > + 			}
   > + 		}
   > + 	}
   > + 	if (num_filenames > 0)
   > + 	{
   > + 		int i;
   > + 		qsort(filenames, num_filenames, sizeof(char *), comparestr);
   > + 		for (i = 0; i < num_filenames; i++)
   > + 		{
   > + 			if (!ParseConfigFile(filenames[i], NULL,
   > + 								 0, elevel,head_p, tail_p))
   > + 			{
   > + 				status = false;
   > + 				goto cleanup;
   > + 			}
   > + 		}
   > + 	}
   > + 	status = true;
   > + 
   > + cleanup:
   > + 	if (num_filenames > 0)
   > + 	{
   > + 		int i;
   > + 
   > + 		for (i = 0; i < num_filenames; i++)
   > + 		{
   > + 			free(filenames[i]);
   > + 		}
   > + 		free(filenames);
   > + 	}
   > + 	FreeDir(d);
   > + 	return status;
   > + }
   
   > *** a/src/include/utils/guc.h
   > --- b/src/include/utils/guc.h
   
   > *************** extern const char *GetConfigOption(const
   > *** 302,307 ****
   > --- 303,313 ----
   >   				bool restrict_superuser);
   >   extern const char *GetConfigOptionResetString(const char *name);
   >   extern void ProcessConfigFile(GucContext context);
   > + extern bool ParseConfigDirectory(const char *includedir,
   > + 						 const char *calling_file,
   > + 						 int depth, int elevel,
   > + 						 ConfigVariable **head_p,
   > + 						 ConfigVariable **tail_p);
   
   This prototype belongs near ParseConfigFile().
   
   Thanks,
   nm
   
   

8. Re: Configuration include directory

   Greg Smith <greg@2ndquadrant.com> — 2011-12-07T11:05:18Z

   On 11/17/2011 11:03 AM, Tom Lane wrote:
   > So as long as the include-directory code path doesn't
   > interfere with tracking that nesting depth, I don't think it needs
   > any extra protection against include-the-same-directory.
   >    
   
   That was the theory in Magnus's original patch, and I don't believe 
   anything has broken that part; I did glance at it.  Since I have a pile 
   of good feedback from Noah now, I'll specifically test this as part of 
   submitting the next patch update.
   
   -- 
   Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
   PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
   
   
   

9. Re: Configuration include directory

   Marti Raudsepp <marti@juffo.org> — 2011-12-07T13:27:55Z

   On Wed, Nov 16, 2011 at 06:53, Greg Smith <greg@2ndquadrant.com> wrote:
   > -Considers all names in that directory that end with *.conf  [Discussion
   > concluded more flexibility here would be of limited value relative to how it
   > complicates the implementation]
   
   I'd suggest also excluding hidden files -- files that start with a
   dot. That's how glob/fnmatch functions work and most "include all
   files" implementations are based on that.
   
   Regards,
   Marti
   
   

10. Re: Configuration include directory

    Greg Smith <greg@2ndquadrant.com> — 2011-12-12T18:34:24Z

    Attached is an update to my earlier patch.  This clears my own bug, 
    usability concerns, and implementation ideas list on this one.
    
    There's full documentation on this now, including some suggested ways 
    all these include features might be used.  Since there's so much 
    controversy around the way I would like to see things organized by 
    default, instead of kicking that off again I just outline a couple of 
    ways people might do things, showing both the flexibility and some good 
    practices I've seen that people can adopt--or not.  Possibilities rather 
    than policy.  The include-related section got big enough that it was 
    really disruptive, so I moved it to a new sub-section.  I like the flow 
    of this better, it slightly slimmed down the too big "Setting 
    Parameters" section.  You can see a snapshot of the new doc page I built 
    at http://http://www.westnet.com/~gsmith/config-setting.html
    
    Here's a partial demo highlighting the updated ignoring logic (which I 
    tested more extensively with some debugging messages about its 
    decisions, then removed those).  Both .02test.conf and .conf appear, but 
    neither are processed:
    
    $ tail -n 1 $PGDATA/postgresql.conf
    include_dir='conf.d'
    $ ls -a $PGDATA/conf.d
    .  ..  00test.conf  01test.conf  .02test.conf  .conf
    $ cat $PGDATA/conf.d/00test.conf
    work_mem = 2MB
    $ cat $PGDATA/conf.d/01test.conf
    work_mem = 3MB
    $ cat $PGDATA/conf.d/.conf
    checkpoint_segments=10
    $ psql -x -c "select name,setting,sourcefile from pg_settings where 
    name='work_mem'"
    -[ RECORD 1 ]---------------------------------------------------
    name       | work_mem
    setting    | 3072
    sourcefile | /home/gsmith/pgwork/data/confdir/conf.d/01test.conf
    $ psql -x -c "select name,setting,sourcefile from pg_settings where 
    name='checkpoint_segments'"
    -[ RECORD 1 ]-------------------
    name       | checkpoint_segments
    setting    | 3
    sourcefile |
    
    In addition to adding the docs, I changed these major things relative to 
    the version that was reviewed:
    
    -The configuration directory name is now considered relative to the 
    directory that the including file is located in.  That's the same logic 
    ParseConfigFile used to convert relative to absolute paths, and as Noah 
    suggested it nicely eliminates several of the concerns I had about what 
    I'd submitted before.  I was concerned before about creating a packaging 
    problem, now I'm not.  Relocate postgresql.conf, and the include 
    directory base goes with it, regardless of the method you used to do 
    so.  The shared logic for this has been refactored into a new 
    AbsoluteConfigLocation function that both ParseConfigFile and this new 
    ParseConfigDirectory call.  That's an improvement to the readability of 
    both functions too.
    
    -With that change, all of the hackery around exporting configdir goes 
    away too.  Which means that the code works as expected during SIGHUP 
    reloads too.  I love it when a plan comes together.
    
    -Hidden files (starts with ".") are now skipped.  This also eliminates 
    the concern about whether ".conf" is considered a valid name for a file; 
    it is clearly not.
    
    -Per renaming suggestion from Robert to make my other submission use 
    include_if_exists, I've made this one include_dir instead of includedir.
    
    There's some new error handling:
    
    -If the configuration directory does not exist at all, throw an error.  
    It was quietly accepted before.  I'm not sure why Magnus had coded it 
    that way originally, and I just passed that through without considering 
    a change.  This still quietly accepts a directory that exists but has no 
    files in it.  I consider that a reasonable behavior, but I wouldn't 
    reject the idea of giving a warning when that happens, if someone feels 
    that's appropriate here.
    
    -When a file that was in the directory goes away before it is checked 
    with stat, consider that an error too.
    
    There are some internal changes that should eliminate the main concerns 
    about Windows compatibility in particular:
    
    -File name joining uses join_path_components, eliminating all sorts of bugs
    -Use pstrdup() instead of strdup when building the list of files in the 
    directory
    -Switch from using guc_realloc to [re]palloc for that temporary file list.
    -Eliminated now unneeded export of guc_malloc and guc_realloc
    -Moved ParseConfigDirectory prototype to the right place
    -Don't bother trying to free individual bits of memory now that it's all 
    in the same context.  Saves some lines of code, and I do not miss the 
    asserts I am no longer triggering.
    
    The only review suggestion I failed to incorporate was this one from Noah:
    
     > > > +         if (strcmp(de->d_name + strlen(de->d_name) - 5, 
    ".conf") != 0)
     > > +             continue;
     > That may as well be memcmp().
    
    While true, his idea bothers both my abstraction and unexpected bug 
    concern sides for reasons I can't really justify.  I seem to have 
    received too many past beatings toward using the string variants of all 
    these functions whenever operating on things that are clearly strings.  
    I'll punt this one toward whoever looks at this next to decide, both 
    strcmp and strncmp are used in this section now.
    
    -- 
    Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
    PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
    
    

11. Re: Configuration include directory

    Greg Smith <greg@2ndquadrant.com> — 2011-12-12T18:40:22Z

    On 12/12/2011 01:34 PM, Greg Smith wrote:
    > You can see a snapshot of the new doc page I built at 
    > http://http://www.westnet.com/~gsmith/config-setting.html
    
    One minute past send note on brain fade:  this section
    
    include '00shared.conf'
    include '01memory.conf'
    include '02server.conf'
    
    
    Was a pasto-o that was supposed to just be a list of files:
    
    00shared.conf
    01memory.conf
    02server.conf
    
    If that's the only thing I'm called on to change, I'll happily update patch and doc sample to reflect it.  It's a trivial and obvious fix to the documentation source.
    
    
    -- 
    Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
    PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
    
    
    

12. Re: Configuration include directory

    Noah Misch <noah@leadboat.com> — 2011-12-13T18:28:49Z

    On Mon, Dec 12, 2011 at 01:34:24PM -0500, Greg Smith wrote:
    
    [various things I agree with]
    
    > -Don't bother trying to free individual bits of memory now that it's all  
    > in the same context.  Saves some lines of code, and I do not miss the  
    > asserts I am no longer triggering.
    
    In the postmaster, this context is the never-reset PostmasterContext.  Thus,
    these yield permanent leaks.  The rest of the ProcessConfigFile() code makes
    an effort to free everything it allocates, so let's do the same here.  (I'd
    favor, as an independent effort, replacing some of the explicit pfree()
    activity in guc-file.l with a temporary memory context create/delete.)
    
    > The only review suggestion I failed to incorporate was this one from Noah:
    >
    > > > > +         if (strcmp(de->d_name + strlen(de->d_name) - 5, ".conf") 
    > != 0)
    > > > +             continue;
    > > That may as well be memcmp().
    >
    > While true, his idea bothers both my abstraction and unexpected bug  
    > concern sides for reasons I can't really justify.  I seem to have  
    > received too many past beatings toward using the string variants of all  
    > these functions whenever operating on things that are clearly strings.   
    > I'll punt this one toward whoever looks at this next to decide, both  
    > strcmp and strncmp are used in this section now.
    
    Okay.
    
    > diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
    > index d1e628f..5df214e 100644
    > *** a/doc/src/sgml/config.sgml
    > --- b/doc/src/sgml/config.sgml
    
    > *************** SET ENABLE_SEQSCAN TO OFF;
    > *** 178,184 ****
    >       any desired selection condition. It also contains more information about
    >       what values are allowed for the parameters.
    >      </para>
    > !   </sect1>
    >   
    >      <sect1 id="runtime-config-file-locations">
    >       <title>File Locations</title>
    > --- 161,273 ----
    >       any desired selection condition. It also contains more information about
    >       what values are allowed for the parameters.
    >      </para>
    > ! 
    > !      <sect2 id="config-includes">
    > !       <title>Configuration file includes</title>
    
    Our section names use title case.
    
    > !        <para>
    > !        <indexterm>
    > !         <primary><literal>include</></primary>
    > !         <secondary>in configuration file</secondary>
    > !        </indexterm>
    > !        In addition to parameter settings, the <filename>postgresql.conf</>
    > !        file can contain <firstterm>include directives</>, which specify
    > !        another file to read and process as if it were inserted into the
    > !        configuration file at this point.  Include directives simply look like:
    > ! <programlisting>
    > ! include 'filename'
    > ! </programlisting>
    > !        If the file name is not an absolute path, it is taken as relative to
    > !        the directory containing the referencing configuration file.
    > !        All types of inclusion directives can be nested.
    > !       </para>
    > ! 
    > !       <para>
    > !        <indexterm>
    > !         <primary><literal>include_dir</></primary>
    > !         <secondary>in configuration file</secondary>
    > !        </indexterm>
    > !        The <filename>postgresql.conf</> file can also contain 
    > !        <firstterm>include_dir directives</>, which specify an entire directory
    > !        of configuration files to include.  It is used similarly:
    > ! <programlisting>
    > ! include_dir 'directory'
    > ! </programlisting>
    > !        Non-absolute directory names follow the same rules as single file include
    > !        directives:  they are relative to the directory containing the referencing
    > !        configuration file.  Within that directory, only files whose names end
    
    Consider the wording "Within that directory, only non-directory files whose
    names ..." to communicate that we ignore all subdirectories.
    
    > !        with the suffix <literal>.conf</literal> will be included.  File names
    > !        that start with the <literal>.</literal> character are also excluded,
    > !        to prevent mistakes as they are hidden on some platforms.  Multiple files
    > !        within an include directory are ordered by an alphanumeric sorting, so
    > !        that ones beginning with numbers are considered before those starting
    > !        with letters.
    > !       </para>
    > ! 
    > !       <para>
    > !       Include files or directories can be used to logically separate portions
    > !       of the database configuration, rather than having a single large
    > !       <filename>postgresql.conf</> file.  Consider a company that has two
    > !       database servers, each with a different amount of memory.  There are likely
    > !       elements of the configuration both will share, for things such as logging.
    > !       But memory-related parameters on the server will vary between the two.  And
    > !       there might be server specific customizations too.  One way to manage this
    
    I suggest the punctuation "server-specific customizations, too."
    
    > !       situation is to break the custom configuration changes for your site into
    > !       three files.  You could add this to the end of your
    > !    <filename>postgresql.conf</> file to include them:
    > ! <programlisting>
    > ! include 'shared.conf'
    > ! include 'memory.conf'
    > ! include 'server.conf'
    > ! </programlisting>
    > !       All systems would have the same <filename>shared.conf</>.  Each server
    > !       with a particular amount of memory could share the same
    > !       <filename>memory.conf</>; you might have one for all servers with 8GB of RAM,
    > !       another for those having 16GB.  And finally <filename>server.conf</> could
    > !       have truly server-specific configuration information in it.
    > !       </para>
    > ! 
    > !       <para>
    > !       Another possibility for this same sort of organization is to create a
    > !       configuration file directory and put this information into files there.
    > !       Other programs such as <productname>Apache</productname> use a
    > !       <filename>conf.d</> directory for this purpose.  And using numbered names
    
    This specific use of conf.d is a distribution-driven pattern; the upstream
    Apache HTTP Server distribution never suggests it directly.
    
    > !       to enforce an ordering is common in UNIX startup script directories named 
    > !       like <filename>/etc/rcN.d</filename>, where <literal>N</> gives the
    > !       associated system runlevel.  Both of these ideas might be combined by
    
    This text suggests to me that the N in question is used for ordering.  It's
    not, though the order of names _inside_ each such directory does drive the
    order of execution.
    
    Overall, I'd probably just remove these comparisons to other projects.
    
    > !       adding a <filename>conf.d</> directory where the
    > !       <filename>postgresql.conf</> file is at, then referencing it at the end:
    > ! <programlisting>
    > ! include_dir 'conf.d'
    > ! </programlisting>
    > !       Then you could name the files in the <filename>conf.d</> directory like this:
    > ! <programlisting>
    > ! include '00shared.conf'
    > ! include '01memory.conf'
    > ! include '02server.conf'
    > ! </programlisting>
    
    This and ...
    
    > !       This shows a clear order in which these files will be loaded.  This is
    > !       important because only the last setting encountered when the server is
    > !       reading its configuration will be used.  Something set in
    > !       <filename>conf.d/02server.conf</> in this example would override a value
    > !       set in <filename>conf.d/01memory.conf</>.
    > !    </para>
    > ! 
    > !    <para>
    > !       You might instead use this configuration directory approach while naming
    > !       these files more descriptively:
    > ! <programlisting>
    > ! 00shared.conf
    > ! 01memory-8GB.conf
    > ! 02server-foo.conf
    > ! </programlisting>
    
    ... this should be a <screen> rather than a <programlisting>.
    
    > !       This sort of arrangement gives a unique name for each configuration file
    > !       variation.  This can help eliminate ambiguity when several servers have
    > !       their configurations all stored in one place, such as in a version
    > !       control repository.  (Storing database configuration files under version
    > !       control is another good practice to consider).
    > !       </para>
    > !      </sect2>
    > !    </sect1>
    >   
    >      <sect1 id="runtime-config-file-locations">
    >       <title>File Locations</title>
    > diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
    > index a094c7a..ae33fdd 100644
    > *** a/src/backend/utils/misc/guc-file.l
    > --- b/src/backend/utils/misc/guc-file.l
    
    > --- 421,427 ----
    >   		return false;
    >   	}
    >   
    > ! 	config_file=AbsoluteConfigLocation(config_file,calling_file);
    
    Fix whitespace.
    
    > *************** ParseConfigFp(FILE *fp, const char *conf
    > *** 599,604 ****
    > --- 627,747 ----
    >   	return OK;
    >   }
    >   
    > + /*
    > +  * String comparison code for use by qsort.  Borrowed from
    > +  * src/backend/tsearch/ts_utils.c
    > +  */
    > + static int
    > + comparestr(const void *a, const void *b)
    > + {
    > + 	return strcmp(*(char **) a, *(char **) b);
    > + }
    > + 
    > + /*
    > +  * Read and parse all config files in a subdirectory in alphabetical order
    > +  */
    > + bool
    > + ParseConfigDirectory(const char *includedir,
    > + 				const char *calling_file,
    > + 				int depth, int elevel,
    > + 				ConfigVariable **head_p,
    > + 				ConfigVariable **tail_p)
    > + {
    > + 	char *directory;
    > + 	DIR *d;
    > + 	struct dirent *de;
    > + 	char **filenames = NULL;
    > + 	int num_filenames = 0;
    > + 	int size_filenames = 0;
    > + 	bool status;
    > + 
    > + 	directory=AbsoluteConfigLocation(includedir,calling_file);
    
    Our normal pgindent runs do not visit flex input files.  Manually run pgindent
    on guc-file.l and merge the reindented versions of ParseConfigDirectory() and
    AbsoluteConfigLocation() into your patch.
    
    > + 	d = AllocateDir(directory);
    > + 	if (d == NULL)
    > + 	{
    > + 		ereport(elevel,
    > + 				(errcode_for_file_access(),
    > + 				 errmsg("could not open configuration directory \"%s\": %m",
    > + 						directory)));
    > + 		return false;
    > + 
    > + 	}
    > + 
    > + 	/*
    > + 	 * Read the directory and put the filenames in an array, so we can sort
    > + 	 * them prior to processing the contents.
    > + 	 */
    > + 	while ((de = ReadDir(d, directory)) != NULL)
    > + 	{
    > + 		struct stat st;
    > + 		char filename[MAXPGPATH];
    > + 
    > + 		/*
    > + 		 * Only parse files with names ending in ".conf".  Explicitly reject
    > + 		 * files starting with ".".  This excludes things like "." and "..", 
    > + 		 * as well as typical hidden files, backup files, and editor debris.
    > + 		 */
    > + 		if (strlen(de->d_name) < 6)
    > + 			continue;
    > + 		if (strncmp(de->d_name, ".", 1) == 0)
    
    I'd write this as "if (de->d_name[0] == '.')", but it's subjective.
    
    > + 			continue;
    > + 		if (strcmp(de->d_name + strlen(de->d_name) - 5, ".conf") != 0)
    > + 			continue;
    > + 
    > + 		join_path_components(filename, directory, de->d_name);
    
    You need a canonicalize_path(), too.  If the original config directory was
    absolute, it has not yet seen canonicalization.
    
    > + 		if (stat(filename, &st) == 0)
    > + 		{
    > + 			if (!S_ISDIR(st.st_mode))
    > + 			{
    > + 				/* Add file to list, increasing its size in blocks of 32 */
    > + 				if (num_filenames == size_filenames)
    > + 				{
    > + 					size_filenames += 32;
    > + 					if (num_filenames == 0)
    > + 						/* Must initialize, repalloc won't take NULL input */
    > + 						filenames = palloc(size_filenames * sizeof(char *));
    > + 					else
    > + 						filenames = repalloc(filenames, size_filenames * sizeof(char *));
    > + 				}
    > + 				filenames[num_filenames] = pstrdup(filename);
    > + 				num_filenames++;
    > + 			}
    > + 		else
    
    This "else" block connects to the wrong "if" block.
    
    > + 			/* 
    > + 			 * stat does not care about permissions, so the most likely reason
    > + 			 * a file can't be accessed now is if it was removed between the
    > + 			 * directory listing and now.
    > + 			 */
    > + 			{
    
    This comment belongs inside the brace.
    
    There are a few other improbable causes.  For one, the postmaster could have
    read permission, but not search permission, on the include dir.
    
    > + 			ereport(elevel,
    > + 					(errcode_for_file_access(),
    > + 					 errmsg("configuration file \"%s\" can no longer be accessed: %m",
    > + 							filename)));
    
    Due to the diversity of possible causes, most of them unlikely, I would use
    "could not stat file \"%s\": %m".  That also avoids adding a string requiring
    translation.
    
    > + 			return false;
    > + 			}
    > + 		}
    > + 	}
    > + 
    > + 	if (num_filenames > 0)
    > + 	{
    > + 		int i;
    > + 		qsort(filenames, num_filenames, sizeof(char *), comparestr);
    > + 		for (i = 0; i < num_filenames; i++)
    > + 		{
    > + 			if (!ParseConfigFile(filenames[i], NULL,
    > + 								 0, elevel,head_p, tail_p))
    
    Need to pass "depth", not 0.  I created a circular include structure and got
    this error instead of the expected one about the nesting limit:
    
    FATAL:  exceeded MAX_ALLOCATED_DESCS while trying to open directory "/home/nm/test-pg/baz"
    
    Thanks,
    nm
    
    

13. Re: Configuration include directory

    Peter Eisentraut <peter_e@gmx.net> — 2011-12-13T20:22:46Z

    On tis, 2011-11-15 at 23:53 -0500, Greg Smith wrote:
    > -Called by specifying "includedir <directory>".  No changes to the 
    > shipped postgresql.conf yet.
    > -Takes an input directory name
    > -If it's not an absolute path, considers that relative to the -D option 
    > (if specified) or PGDATA, the same logic used to locate the 
    > postgresql.conf (unless a full path to it is used)
    > -Considers all names in that directory that end with *.conf  [Discussion 
    > concluded more flexibility here would be of limited value relative to 
    > how it complicates the implementation]
    > -Loops over the files found in sorted order by name
    
    > I can see some potential confusion here in one case.  Let's say someone 
    > specifies a full path to their postgresql.conf file.  They might assume 
    > that the includedir was relative to the directory that file is in.  
    > Let's say configfile is /etc/sysconfig/pgsql/postgresql.conf ; a user 
    > might think that "includedir conf.d" from there would reference 
    > /etc/sysconfig/pgsql/conf.d instead of the $PGDATA/conf.d you actually 
    > get.  Wavering on how to handle that is one reason I didn't try 
    > documenting this yet, the decision I made here may not actually be the 
    > right one.
    
    Well, the existing include directive works relative to the directory the
    including file is in.  If includedir works differently from that, that
    would be highly confusing.
    
    I would actually just extend "include" to accept wildcards instead of
    inventing a slightly new and slightly different mechanism.
    
    
    

14. Re: Configuration include directory

    Greg Smith <greg@2ndquadrant.com> — 2011-12-13T21:55:14Z

    On 12/13/2011 01:28 PM, Noah Misch wrote:
    >> !<para>
    >> !       Another possibility for this same sort of organization is to create a
    >> !       configuration file directory and put this information into files there.
    >> !       Other programs such as<productname>Apache</productname>  use a
    >> !<filename>conf.d</>  directory for this purpose.  And using numbered names
    >>      
    > This specific use of conf.d is a distribution-driven pattern; the upstream
    > Apache HTTP Server distribution never suggests it directly...
    > ...
    >
    > Overall, I'd probably just remove these comparisons to other projects.
    >    
    
    I hadn't realized that distinction; will have to look into that some 
    more.  Thanks again for the thorough review scrubbings, I can see I have 
    another night of getting cozy with mmgr/README ahead.  I've gotten more 
    than a fair share of feedback time for this CF, I'm going to close this 
    patch for now, keep working on it for a bit more, and re-submit later.
    
    My hope with this new section is that readers will realize the 
    flexibility and options possible with the include and include_dir 
    commands, and inspire PostgreSQL users to adopt familiar conventions 
    from other programs if they'd like to.  I've made no secret of the fact 
    that I don't like the way most people are led toward inefficiently 
    managing their postgresql.conf files, that I feel the default 
    configurations both encourages bad practices and makes configuration 
    tool authoring a mess.  I would really like to suggest some possible 
    alternatives here and get people to consider them, see if any gain 
    adoption.  I thought that mentioning the examples are inspired by common 
    setups of other programs, ones that people are likely to be familiar 
    with, enhanced that message.  That's not unprecedented; 
    doc/src/sgml/client-auth.sgml draws a similar comparison with Apache in 
    regards to how parts of the pg_hba.conf are configured.  No argument 
    here that I need to clean that section up still if I'm going to make 
    this argument though.  I didn't expect to throw out 85 new lines of docs 
    and get them perfect the first time.
    
    -- 
    Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
    PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
    
    
    

15. Re: Configuration include directory

    Greg Smith <greg@2ndquadrant.com> — 2011-12-13T23:19:47Z

    On 12/13/2011 03:22 PM, Peter Eisentraut wrote:
    > Well, the existing include directive works relative to the directory the
    > including file is in.  If includedir works differently from that, that
    > would be highly confusing.
    >    
    
    Right, and that's gone now; latest update matches the regular include 
    behavior.
    
    > I would actually just extend "include" to accept wildcards instead of
    > inventing a slightly new and slightly different mechanism.
    >    
    
    That's one of the ideas thrown out during the first round of discussion 
    around this patch.  Tom's summary of why that wasn't worth doing hits 
    the highlights:  
    http://archives.postgresql.org/pgsql-hackers/2009-10/msg01628.php
    
    -- 
    Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
    PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
    
    
    

16. Re: Configuration include directory

    Selena Deckelmann <selena@chesnok.com> — 2012-09-20T21:10:58Z

    Hello!
    
    I've spent a little time with this patch and have attached revision 6.
     Thanks, Noah, for a fantastically detailed review.
    
    The only thing I didn't do that Noah suggested was run pgindent on
    guc-file.l. A cursory search did not reveal source compatible with my
    operating system for 'indent'. If someone points me to it, I'd happily
    also comply with the request to reindent. And document how to do that
    on my platform(s). :)
    
    I did just remove the references to the Apache project etc. I agree
    that providing best practices is good, but I'm skeptical about
    including best practices piecemeal. Adding it to earlier tutorial
    sections would probably be a bit more visible IMO.
    
    I also added examples to postgresql.conf.sample, per a suggestion from
    Dave Page.
    
    -selena
    
    -- 
    http://chesnok.com
    

17. Re: Configuration include directory

    Heikki Linnakangas <hlinnakangas@vmware.com> — 2012-09-24T08:41:01Z

    On 21.09.2012 00:10, Selena Deckelmann wrote:
    > Hello!
    >
    > I've spent a little time with this patch and have attached revision 6.
    >   Thanks, Noah, for a fantastically detailed review.
    >
    > The only thing I didn't do that Noah suggested was run pgindent on
    > guc-file.l. A cursory search did not reveal source compatible with my
    > operating system for 'indent'. If someone points me to it, I'd happily
    > also comply with the request to reindent. And document how to do that
    > on my platform(s). :)
    >
    > I did just remove the references to the Apache project etc. I agree
    > that providing best practices is good, but I'm skeptical about
    > including best practices piecemeal. Adding it to earlier tutorial
    > sections would probably be a bit more visible IMO.
    
    This seems pretty much ready to commit. One tiny detail that I'd like to 
    clarify: the docs say:
    
    > Multiple files within an include directory are ordered by an alphanumeric sorting, so that ones beginning with numbers are considered before those starting with letters.
    
    To be more precise, the patch uses strcmp() for the comparisons. That's 
    also what apache seems to do, although I couldn't find it being 
    mentioned explicitly in their docs. It's true that numbers are sorted 
    before letters, but should we also mention that upper-case letters are 
    sorted before lower-case ones, and that sorting of non-ASCII characters 
    depends on the encoding, in often surprising ways? Is there a better 
    term for what strcmp() does? ASCII order? Is there precedence somewhere 
    else in the PostgreSQL codebase or docs for that?
    
    - Heikki
    
    
    

18. Re: Configuration include directory

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-09-24T14:24:56Z

    Heikki Linnakangas <hlinnakangas@vmware.com> writes:
    > This seems pretty much ready to commit. One tiny detail that I'd like to 
    > clarify: the docs say:
    
    >> Multiple files within an include directory are ordered by an alphanumeric sorting, so that ones beginning with numbers are considered before those starting with letters.
    
    > To be more precise, the patch uses strcmp() for the comparisons.
    
    Just say it sorts the file names according to C locale rules.
    
    			regards, tom lane
    
    
    

19. Re: Configuration include directory

    Heikki Linnakangas <hlinnakangas@vmware.com> — 2012-09-24T14:41:03Z

    On 24.09.2012 17:24, Tom Lane wrote:
    > Heikki Linnakangas<hlinnakangas@vmware.com>  writes:
    >> This seems pretty much ready to commit. One tiny detail that I'd like to
    >> clarify: the docs say:
    >
    >>> Multiple files within an include directory are ordered by an alphanumeric sorting, so that ones beginning with numbers are considered before those starting with letters.
    >
    >> To be more precise, the patch uses strcmp() for the comparisons.
    >
    > Just say it sorts the file names according to C locale rules.
    
    Hmm, that's preceise, but I don't think an average user necessarily 
    knows what the C locale is. I think I'll go with:
    
    Multiple files within an include directory are processed in filename 
    order. The filenames are ordered by C locale rules, ie. numbers before 
    letters, and uppercase letters before lowercase ones.
    
    - Heikki
    
    
    

20. Re: Configuration include directory

    Gavin Flower <gavinflower@archidevsys.co.nz> — 2012-09-24T19:13:59Z

    On 25/09/12 02:41, Heikki Linnakangas wrote:
    > On 24.09.2012 17:24, Tom Lane wrote:
    >> Heikki Linnakangas<hlinnakangas@vmware.com>  writes:
    >>> This seems pretty much ready to commit. One tiny detail that I'd 
    >>> like to
    >>> clarify: the docs say:
    >>
    >>>> Multiple files within an include directory are ordered by an 
    >>>> alphanumeric sorting, so that ones beginning with numbers are 
    >>>> considered before those starting with letters.
    >>
    >>> To be more precise, the patch uses strcmp() for the comparisons.
    >>
    >> Just say it sorts the file names according to C locale rules.
    >
    > Hmm, that's preceise, but I don't think an average user necessarily 
    > knows what the C locale is. I think I'll go with:
    >
    > Multiple files within an include directory are processed in filename 
    > order. The filenames are ordered by C locale rules, ie. numbers before 
    > letters, and uppercase letters before lowercase ones.
    >
    > - Heikki
    >
    >
    Even I can understand that!  :-)
    
    More to the point: are fullstops '.' sorted before digits?
    
    
    Cheers,
    Gavin
    
    
    
    

21. Re: Configuration include directory

    Noah Misch <noah@leadboat.com> — 2012-09-24T19:56:48Z

    On Thu, Sep 20, 2012 at 02:10:58PM -0700, Selena Deckelmann wrote:
    > The only thing I didn't do that Noah suggested was run pgindent on
    > guc-file.l. A cursory search did not reveal source compatible with my
    > operating system for 'indent'. If someone points me to it, I'd happily
    > also comply with the request to reindent. And document how to do that
    > on my platform(s). :)
    
    For future reference, src/tools/pgindent/README points to the pg_bsd_indent
    sources.  If pg_bsd_indent fails to build and run, post the details.
    
    
    

22. Re: Configuration include directory

    Heikki Linnakangas <hlinnakangas@vmware.com> — 2012-09-25T07:37:29Z

    On 24.09.2012 22:13, Gavin Flower wrote:
    > On 25/09/12 02:41, Heikki Linnakangas wrote:
    >> Multiple files within an include directory are processed in filename
    >> order. The filenames are ordered by C locale rules, ie. numbers before
    >> letters, and uppercase letters before lowercase ones.
    >
    > Even I can understand that! :-)
    >
    > More to the point: are fullstops '.' sorted before digits?
    
    Yes. But files beginning with a fullstop are ignored.
    
    - Heikki