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

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.

The permission checking is faulty, because unprivileged users can
execute pg_hba_settings() directly.

Check the error messages against the style guide (especially
capitalization).

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.

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).

I would put the line_number column first.

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.

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.

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).

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 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.