Re: [HACKERS] pg_hba_file_settings view patch

On Thu, Jan 5, 2017 at 1:58 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Could you hold on a bit to commit that? I'd like to look at it in more
> details. At quick glance, there is for example no need to use
> CreateTemplateTupleDesc and list the columns both in pg_proc.h and the
> C routine itself. And memset() can be used in fill_hba_line for the
> error code path.

And here we go.

+<programlisting>
+postgres=# select * from pg_hba_rules;
[... large example ...]
+
+</programlisting>
It would be nice to reduce the width of this example. That's not going
to be friendly with the generated html.

+       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");
+               break;
+       }
Here let's remove the break clause and let compilers catch problem
when they show up.

+   if (hba->pamservice)
+   {
+       initStringInfo(&str);
+       appendStringInfoString(&str, "pamservice=");
+       appendStringInfoString(&str, hba->pamservice);
+       options[noptions++] = CStringGetTextDatum(str.data);
+   }
There is a bunch of code duplication here. I think that it would make
more sense to encapsulate that into a routine, at least let's use
appendStringInfo and let's group the two calls together.

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

=# \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 have copied the example file of pg_hba.conf, reloaded it:
https://www.postgresql.org/docs/devel/static/auth-pg-hba-conf.html
And then the output result gets corrupted by showing up free()'d results:
null   | null    | \x7F\x7F\x7F\x7F\x7F

+   if (err_msg)
+   {
+       /* type */
+       index++;
+       nulls[index] = true;
[... long sequence ...]
Please let's use MemSet here and remove this large chunk of code...

+   if (!superuser())
+       ereport(ERROR,
+               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                (errmsg("must be superuser to view pg_hba.conf settings"))));
Access to the function is already revoked, so what's the point of this
superuser check?

+   tupdesc = CreateTemplateTupleDesc(NUM_PG_HBA_LOOKUP_ATTS, false);
+   TupleDescInitEntry(tupdesc, (AttrNumber) 1, "line_number",
+                      INT4OID, -1, 0);
There is no need to list all the columns here. You can just use
get_call_result_type() and be done with it as the types and columns
names are already listed in the pg_proc entry of the new function.
-- 
Michael