Re: pg_hba_file_settings view patch

On Thu, Jan 5, 2017 at 1:58 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)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