Re: Configuration include directory - Mailing list pgsql-hackers

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