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

On Monday, March 04, 2013 8:46 PM Amit Kapila wrote:
> On Monday, March 04, 2013 6:38 AM Greg Smith wrote:
> > 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.
> 
> Thank you for review.

Fujii Masao has also done the review and test of patch multiple times.

> > 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.
> 
> I have tried to completely handle all the comments, but need 1 more
> day.
> 
> > = 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.
> 
> We can name the directory as config.
> For file, I am awaiting your reponse to select from below options
> a. persistent.auto.conf
> b. persist.auto.conf
> c. auto.persist.conf
> d. postgresql.auto.conf

In v11 patch, I have changed name of directory to config.
For file name, currently I have not changed, but if you feel it needs to be
changed, kindly suggest any one of above or if any other better you have in
mind.

> > 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.
> 
> This is for logic to see if variable already exist just replace it's
> value
> at the same
> location in .auto file, else append at end.
> IIUC what you are suggesting is set first in memory then while writing
> just
> check the memory, whatever
> Is new value just write it.
> if we just traverse in memory how will we know where this exist in
> file.
> This might have complication what if write to memory is success, but to
> file
> it fails.
> 
> > 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?
> 
> Today I tried again by having single common function for both places,
> but
> It is getting clumsy. So I can to write multiple functions for each
> datatype?

Finally, I could come up with unified function with common parts together
based on
your suggestions.

> > 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.
> 
> I will do pgindent.
> Can you point what are extra lines that can be removed.

I think it is mainly due to the changes done in gram.y.
> > = 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
> 
> I shall take care of these, minor code related issues due to recent
> changes
> in HEAD.

Gram.y - PERSISTENT word is missed from one of the list
Initdb.c - structure subdirs, doesn't contain new directory.
Fixed both of the above.

> > 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.
> 
> pg_reload_conf(), doesn't ensure that configuration has been propagated
> to
> memory.
> It just sends the SIGHUP signal to postmaster.
> If I remove, it will fail the tests.

I have kept this intact.

With Regards,
Amit Kapila.