Re: pg_hba_file_settings view patch

On Thu, Jan 19, 2017 at 11:28 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> On Thu, Jan 19, 2017 at 1:26 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Thu, Jan 19, 2017 at 4:25 PM, Haribabu Kommi
> > <kommi(dot)haribabu(at)gmail(dot)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.
>

Thanks for the review.

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

Changed accordingly.

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

Added more rows with different options.

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

Thanks for the improvement suggestion.
I am thinking of whether is it really required, as because we rarely change,
the name of authentication option that is already exposed and also added new
options can easily found by the compiler in case if it is missed to add.

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

No. In case if user provides more than 12 options that are invalid, during
the parsing
itself, it identifies that it is an invalid option and error string is
stored in error filed.

The Assert case can be hit only, when the user added to new options to
display
to the user through view but not updating the macro to the max number of
options
then, it can lead to that assert.

Updated patch attached including reverting of file leak changes.

Regards,
Hari Babu
Fujitsu Australia