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

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