Re: Providing catalog view to pg_hba.conf file - Patch submission

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(dot)haribabu(at)gmail(dot)com>:

> On Fri, Mar 13, 2015 at 1:33 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On 3/4/15 1:34 AM, Haribabu Kommi wrote:
> >> On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi
> >> <kommi(dot)haribabu(at)gmail(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>