Re: [HACKERS] pg_hba_file_settings view patch

On Thu, Jan 19, 2017 at 1:26 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jan 19, 2017 at 4:25 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> Added the cleanup mechanism. But the tokenize_file() function call
>> present in many places, But in one flow still it is possible to have
>> file descriptor leak because of pg_hba_rules view. Because of this
>> reason, added the cleanup everywhere.
>
> Oops, sorry. Actually you don't need that. AllocateFile() registers
> the fd opened with the sub-transactions it is involved with... So if
> there is an ERROR nothing leaks.

I agree. If we need any fix, it should be a separate patch.

The patch is in much better shape than previous versions. Thanks for
working on it.

Here are some more review comments.
'indicates' should be used instead of 'indicating'
+  <para>
+   If the configuration file contains any problems,
<structfield>error</structfield> field
+   indicating the problem of that rule. Following is the sample
output of the view.
+  </para>
The first sentence may be rewritten as
<structfield>error</structfield> field, if not NULL, describes problem in
the rule on the line <structfield>line_number</structfield>.

Instead of showing same values like {all}, trust on multiple lines, you may
show an example with different values on different lines.
+<screen>
+ line_number | type  | database | user_name | auth_method
+-------------+-------+----------+-----------+-------------
+          84 | local | {all}    | {all}     | trust
+          86 | host  | {all}    | {all}     | trust
+          88 | host  | {all}    | {all}     | trust
+(3 rows)
+</screen>

getauthmethod() deparses the authentication tokens parsed in parse_hba_line()
starting with /* Get the authentication method */. There is less chance that
those tokens would be changed later, but we might need adjustments when new
methods are added or method names are changed. Instead, we might want to create
an array of token where nth token indicates auth_method = n. The code block in
parse_hba_line() can be changed to look up this array and assign index of the
token if found to auth_method. Token which are enabled by compiler flags will
be part of the array only when that flag is enabled, otherwise they will be
NULL.
#ifdef ENABLE_GSS
        parsedline->auth_method = uaGSS;
#else
        unsupauth = "gss";
#endif
If we do that getauthmethod() simply fetches the token by referencing array
with auth_method as index, with some special handling for uaImplicitReject.
This will take away any future maintenance needed. Something similar can be
done to conntype.

This is not going to help in binary without CASSERT i.e. for most users, if
they provide more than 12 options, albeit resulting in an error. Please convert
this into an elog() or another error that hba parser throws.
+    Assert(noptions <= MAX_OPTIONS);