Re: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review] - Mailing list pgsql-hackers

This submission didn't have any listed reviewers in the CF app.  I added 
Zoltan and Andres since both of you went through the usual review steps 
and have given lots of feedback.

There are two main sets of issues that keep popping up with this feature:

-The implementation seems too long, at around 2189 lines of diff.  I 
have a few ideas for how things might be trimmed below.  I do appreciate 
that a good part of the length is a long set of regression tests, 
relative to what a feature like this normally gets.

-It might be possible to get a simpler implementation with a file per 
setting.

I can make a pass over cleaning up the wording in your comments and 
documentation.  There are enough coding issues that I think that should 
wait until another rev of the patch first.

= Directory vs. single file =

The main reason I've advocated a configuration file directory is to try 
and suggest a standard for tool generated config files to co-exist in. 
This particular feature doesn't *need* that.  But in the long term I was 
hoping to have more tools that can write out .conf files without having 
to read and understand every existing file that's in there.  It doesn't 
make that big of a difference whether this particular one lives in 
$PGDATA/postgresql.auto.conf or $PGDATA/postgresql.auto.conf.  The main 
reason for the directory is to make the second such tool not further 
clutter the main $PGDATA directory.

I would like to see the name of the directory be conf.d instead of 
auto.conf.d though.  What's "auto" about it?  Using that word just adds 
a source of confusion.  The same problem exists with the file name 
itself.  I was hoping for conf.d/persistent.conf here, and no use of the 
word "auto" in the code itself.

What I don't see a lot of value in is the config file per setting idea.  I was hoping SET PERSISTENT put all of its
changesinto once place. 
 
It should be obvious where they came from, and that people can't edit 
that file manually without breaking the feature.  To me something like 
persistent.conf gives that impression, while postgresql.auto.conf sounds 
like the output from some auto-tuning tool.

= Length reduction =

I'm not sure why you are opening the old auto config file with 
ParseConfigFp.  Can't you just navigate the existing GUCs in memory and 
directly write the new one out?  If someone is going to manually edit 
this file and use SET PERSISTENT, they're going to end up in trouble 
regardless.  I don't think it's really worth the extra complexity needed 
to try and handle that case.

It sounds like you've thought a good bit about how to eliminate all the 
code in the validate_conf_option function.  I think that idea may take 
some more thinking to try and be creative about it.  Have you looked at 
whether there's any way to refactor this to be smaller by modifying 
guc.c to break out parts of its implementation needed here?

Is it possible to modify your changes to src/backend/parser/gram.y to 
produce a smaller diff?  There looks to be a good amount of code (~25 
lines) that aren't really changed that I can see.  I suspect they might 
have been hit with an editor whitespace change.

= General code issues =

There is some trivial bit rot on your v10 here:

1 out of 8 hunks FAILED -- saving rejects to file 
src/backend/parser/gram.y.rej
1 out of 3 hunks FAILED -- saving rejects to file 
src/bin/initdb/initdb.c.rej

Your change to gram.y was broken by PROGRAM being added.  I'm not sure 
why the initdb.c one doesn't apply; whitespace issue maybe?  As written 
right now, even with fixing those two, I'm getting this compiler error:

gram.y:12694.27-36: warning: type clash on default action: <keyword> != <>
gram.y:1306.31-40: symbol PERSISTENT is used, but is not defined as a 
token and has no rules

This combination in src/test/regress/sql/persistent.sql a few times 
makes me nervous:

+ select pg_reload_conf();
+ select pg_sleep(1);

Why the pause?  There are some regression tests with sleeps in them, 
such as the stats collector and the timezone tests.  I would expect 
pg_reload_conf would not return until its configuration is stable, so 
this is a bit of a surprise.

-- 
Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com