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. > 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 > 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? > 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. > = 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. > 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. With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
