Re: pg_hba_file_settings view patch

On Tue, Jan 17, 2017 at 5:24 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Tue, Jan 17, 2017 at 10:19 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > On Tue, Jan 10, 2017 at 6:35 PM, Michael Paquier <
> michael.paquier@gmail.com>
> > wrote:
> >> +/* LDAP supports 10 currently, keep this well above the most any
> >> method needs */
> >> +#define MAX_OPTIONS 12
> >> Er, why? There is an assert already, that should be enough.
> >
> >
> > Which Assert? This macro is used to verify that the maximum number
> > of authentication options that are possible for a single hba line.
>
> That one:
> +   Assert(noptions <= MAX_OPTIONS);
> +   if (noptions)
> +       return PointerGetDatum(
> +               construct_array(options, noptions, TEXTOID, -1, false,
> 'i'));


Sorry, I didn't clearly understand of your comment. The MAX_OPTIONS
macro is used to fill the Datum array to generate the options text array
data.


> >> =# \d pg_hba_rules
> >>                View "pg_catalog.pg_hba_rules"
> >>       Column      |  Type   | Collation | Nullable | Default
> >> ------------------+---------+-----------+----------+---------
> >>  line_number      | integer |           |          |
> >>  type             | text    |           |          |
> >>  keyword_database | text[]  |           |          |
> >>  database         | text[]  |           |          |
> >>  keyword_user     | text[]  |           |          |
> >>  user_name        | text[]  |           |          |
> >>  keyword_address  | text    |           |          |
> >>  address          | inet    |           |          |
> >>  netmask          | inet    |           |          |
> >>  hostname         | text    |           |          |
> >>  method           | text    |           |          |
> >>  options          | text[]  |           |          |
> >>  error            | text    |           |          |
> >> keyword_database and database map actually to the same thing if you
> >> refer to a raw pg_hba.conf file because they have the same meaning for
> >> user. You could simplify the view and just remove keyword_database,
> >> keyword_user and keyword_address. This would simplify your patch as
> >> well with all hte mumbo-jumbo to see if a string is a dedicated
> >> keyword or not. In its most simple shape pg_hba_rules should show to
> >> the user as an exact map of the entries of the raw file.
> >
> > I removed keyword_database and keyword_user columns where the data
> > in those columns can easily represent with the database and user columns.
> > But for address filed can contains keywords such as "same host" and etc
> and
> > also a hostname also. Because of this reason, this field is converted
> into
> > 3 columns in the view.
>
> Hm. We could as well consider cidr and use just one column. But still,
> the use of inet as a data type in a system view looks like a wrong
> choice to me. Or we could actually just use text... Opinions from
> others are welcome here of course.
>

Changed to text datatype and merged address, keyword_address and hostname
into address column. The netmask is the extra column in the view.


> > Updated patch attached.
>
> This begins to look good. I have found a couple of minor issues.
>
> +  <para>
> +   The <structname>pg_hba_rules</structname> view can be read only by
> +   superusers.
> +  </para>
> This is not true anymore.
>

removed.

+     <entry>
> +      Line number within client authentication configuration file
> +      the current value was set at
> +     </entry>
> I'd tune that without a past sentence. Saying just pg_hba.conf would
> be fine perhaps?
>

changed to - "Line number of the client authentication rule in pg_hba.conf
file"


> +    <row>
> +     <entry><structfield>database</structfield></entry>
> +     <entry><structfield>text[]</structfield></entry>
> +     <entry>List of database name</entry>
> +    </row>
> This should be plural, database nameS.
>

corrected.

+     <entry>
> +      List of keyword address names,
> +      name can be all, samehost and samenet
> +     </entry>
> Phrasing looks weird to me, what about "List of keyword address names,
> whose values can be all, samehost or samenet", with <literal> markups.
>

corrected.

+postgres=# select line_number, type, database, user_name, auth_method
> from pg_hba_rules;
> Nit: this could be upper-cased.
>

corrected.

+static Datum
> +getauthmethod(UserAuth auth_method)
> +{
> + ...
> +       default:
> +           elog(ERROR, "unexpected authentication method in parsed HBA
> entry");
> +           break;
> +   }
> I think that you should also remove the default clause here to catchup
> errors at compilation.
>

removed.


> +       switch (hba->conntype)
> +       {
> +           case ctLocal:
> +               values[index] = CStringGetTextDatum("local");
> +               break;
> +           case ctHost:
> +               values[index] = CStringGetTextDatum("host");
> +               break;
> +           case ctHostSSL:
> +               values[index] = CStringGetTextDatum("hostssl");
> +               break;
> +           case ctHostNoSSL:
> +               values[index] = CStringGetTextDatum("hostnossl");
> +               break;
> +           default:
> +               elog(ERROR, "unexpected connection type in parsed HBA
> entry");
> +       }
> You could go without the default clause here as well.
>

removed.

updated patch attached.
Added tap tests patch also attached.

Regards,
Hari Babu
Fujitsu Australia