Обсуждение: 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@2ndQuadrant.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@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
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
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
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@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
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
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
Вложения
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 trivialand obvious fix to the documentation source. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.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@2ndQuadrant.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@2ndQuadrant.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 numbersare 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@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 numbersare 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@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 numbersare 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@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
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