Re: pg_hba_file_settings view patch

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

> On Tue, Jan 17, 2017 at 10:19 AM, Haribabu Kommi
> <kommi(dot)haribabu(at)gmail(dot)com> wrote:
> > On Tue, Jan 10, 2017 at 6:35 PM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)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