Re: Configuration include directory

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