Re: [HACKERS] Configuration include directory

2012-09-25 Thread Heikki Linnakangas

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2012-09-24 Thread Heikki Linnakangas

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2012-09-24 Thread Tom Lane
Heikki Linnakangas hlinnakan...@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.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2012-09-24 Thread Heikki Linnakangas

On 24.09.2012 17:24, Tom Lane wrote:

Heikki Linnakangashlinnakan...@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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2012-09-24 Thread Gavin Flower

On 25/09/12 02:41, Heikki Linnakangas wrote:

On 24.09.2012 17:24, Tom Lane wrote:

Heikki Linnakangashlinnakan...@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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2012-09-24 Thread Noah Misch
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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2012-09-20 Thread Selena Deckelmann
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


config-directory-v6.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-12-13 Thread Noah Misch
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
   titleFile 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
 !   titleConfiguration file includes/title

Our section names use title case.

 !para
 !indexterm
 ! primaryliteralinclude//primary
 ! secondaryin configuration file/secondary
 !/indexterm
 !In addition to parameter settings, the filenamepostgresql.conf/
 !file can contain firstterminclude 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
 ! primaryliteralinclude_dir//primary
 ! secondaryin configuration file/secondary
 !/indexterm
 !The filenamepostgresql.conf/ file can also contain 
 !firstterminclude_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
 !   filenamepostgresql.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
 !filenamepostgresql.conf/ file to include them:
 ! programlisting
 ! include 'shared.conf'
 ! include 'memory.conf'
 ! include 'server.conf'
 ! 

Re: [HACKERS] Configuration include directory

2011-12-13 Thread Peter Eisentraut
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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-12-13 Thread Greg Smith

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 asproductnameApache/productname  use a
!filenameconf.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 USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-12-13 Thread Greg Smith

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 USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-12-12 Thread Greg Smith
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 

Re: [HACKERS] Configuration include directory

2011-12-12 Thread Greg Smith

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 USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-12-07 Thread Greg Smith

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 USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-12-07 Thread Marti Raudsepp
On Wed, Nov 16, 2011 at 06:53, Greg Smith g...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-12-05 Thread Noah Misch
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.

 +   

Re: [HACKERS] Configuration include directory

2011-11-17 Thread Alvaro Herrera

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-11-17 Thread Tom Lane
Alvaro Herrera alvhe...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-11-16 Thread Greg Jaskiewicz

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. 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-11-16 Thread Andrew Dunstan
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Configuration include directory

2011-11-16 Thread Tom Lane
Andrew Dunstan and...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers