Re: Configuration include directory

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

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.

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

"Andrew Dunstan" <andrew(at)dunslane(dot)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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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

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

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

On Wed, Nov 16, 2011 at 06:53, Greg Smith <greg(at)2ndquadrant(dot)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

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

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

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

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.

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

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

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

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

Heikki Linnakangas <hlinnakangas(at)vmware(dot)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

On 24.09.2012 17:24, Tom Lane wrote:
> Heikki Linnakangas<hlinnakangas(at)vmware(dot)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

On 25/09/12 02:41, Heikki Linnakangas wrote:
> On 24.09.2012 17:24, Tom Lane wrote:
>> Heikki Linnakangas<hlinnakangas(at)vmware(dot)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

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.

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