Hi I checked this patch. I like the functionality and behave.
There is minor issue with outdated regress test test rules ... FAILED I have no objections. Regards Pavel 2015-03-27 9:23 GMT+01:00 Haribabu Kommi <kommi.harib...@gmail.com>: > On Fri, Mar 13, 2015 at 1:33 PM, Peter Eisentraut <pete...@gmx.net> wrote: > > On 3/4/15 1:34 AM, Haribabu Kommi wrote: > >> On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi > >> <kommi.harib...@gmail.com> wrote: > >>> + foreach(line, parsed_hba_lines) > >>> > >>> In the above for loop it is better to add "check_for_interrupts" to > >>> avoid it looping > >>> if the parsed_hba_lines are more. > >> > >> Updated patch is attached with the addition of check_for_interrupts in > >> the for loop. > > > > I tried out your latest patch. I like that it updates even in running > > sessions when the file is reloaded. > > Thanks for the review. Sorry for late reply. > > > The permission checking is faulty, because unprivileged users can > > execute pg_hba_settings() directly. > > corrected. > > > Check the error messages against the style guide (especially > > capitalization). > > corrected. > > > I don't like that there is a hard-coded limit of 16 options 5 pages away > > from where it is actually used. That could be done better. > > changed to 12 instead of 16. > > > I'm not sure about the name "pg_hba_settings". Why not just "pg_hba" or > > "pg_hba_conf" if you're going for a representation that is close to the > > file (as opposed to pg_settings, which is really a lot more abstract > > than any particular file). > > changed to pg_hba_conf. > > > I would put the line_number column first. > > changed. > > > I continue to think that it is a serious mistake to stick special values > > like 'all' and 'replication' into the arrays without additional > > decoration. That makes this view useless or at least dangerous for > > editing tools or tools that want to reassemble the original file. > > Clearly at least one of those has to be a use case. Otherwise we can > > just print out the physical lines without interpretation. > > It is possible to provide more than one keyword for databases or users. > Is it fine to use the text array for keyword databases and keyword users. > > > The "mask" field can go, because address is of type inet, which can > > represent masks itself. (Or should it be cidr then? Who knows.) The > > preferred visual representation of masks in pg_hba.conf has been > > "address/mask" for a while now, so we should preserve that. > > Additionally, you can then use the existing inet/cidr operations to do > > things like checking whether some IP address is contained in an address > > specification. > > removed. > > > I can't tell from the documentation what the "compare_method" field is > > supposed to do. I see it on the code, but that is not a natural > > representation of pg_hba.conf. In fact, this just supports my earlier > > statement. Why are special values in the address field special, but not > > in the user or database fields? > > > > uaImplicitReject is not a user-facing authentication method, so it > > shouldn't be shown (or showable). > > removed. > > > I would have expected options to be split into keys and values. > > > > All that code to reassemble the options from the parsed struct > > representation seems crazy to me. Surely, we could save the strings as > > we parse them? > > I didn't get this point clearly. Can you explain it a bit more. > > > I can't make sense of the log message "pg_hba.conf not reloaded, > > pg_hba_settings may show stale information". If load_hba() failed, the > > information isn't stale, is it? > > > > In any case, printing this to the server log seems kind of pointless, > > because someone using the view is presumably doing so because they can't > > or don't want to read the server log. The proper place might be a > > warning when the view is actually called. > > Changed accordingly as if the reload fails, further selects on the > view through a warning > as view data may not be proper like below. > > "There was some failure in reloading pg_hba.conf file. The pg_hba.conf > settings data may contains stale information" > > Here I attached latest patch with the fixes other than keyword columns. > I will provide the patch with keyword columns and documentation changes > later. > > Regards, > Hari Babu > Fujitsu Australia > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
