Re: [HACKERS] pg_hba_file_settings view patch

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

>> =# \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.

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

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

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

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

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

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

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