Re: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

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 changes into 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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com