Re: Configuration include directory - Mailing list pgsql-hackers

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