Re: Providing catalog view to pg_hba.conf file - Patch submission

Hi Vaishnavi.

In addition to Jaime's comments about the functionality, here are are
some comments about the code.

Well, they were supposed to be comments about the code, but it turns out
I have comments about the feature as well. We need to figure out what to
do about the database and user columns. Returning an array containing
e.g. "{all}" won't fly. We must distinguish between "all" and "{all}".

I don't have a good solution, other than returning two columns each: one
a string, and one an array, only one of them set for any given record.

> + int
> + GetNumHbaLines(void)
> + {

Please add a blank line before this.

> + /*
> + * Fetches the HbaLine corresponding to linenum variable.
> + * Fill the values required to build a tuple, of view pg_hba_settings, in values pointer.
> + */
> + void
> + GetHbaLineByNum(int linenum, const char **values)
> + {

I suggest naming this function GetHbaValuesByLinenum() and changing the
comment to "Fill in suitable values to build a tuple representing the
HbaLine corresponding to the given linenum".

Actually, maybe it should be hba_getvaluesbyline() for consistency with
the other functions in the file? In that case, the preceding function
should also be renamed to hba_getnumlines().

> + /* Retrieving connection type */
> + switch (hba->conntype)
> + {

The comment should be just "Connection type". Generally speaking, all of
the "Retrieving" comments should be changed similarly. Or just removed.

> + case ctLocal:
> + values[0] = pstrdup("Local");
> + break;

I agree with Jaime: this should be lowercase. And do you really need to
pstrdup() these strings?

> + appendStringInfoString(&str, "{");
> + foreach(dbcell, hba->databases)
> + {
> + tok = lfirst(dbcell);
> + appendStringInfoString(&str, tok->string);
> + appendStringInfoString(&str, ",");
> + }
> +
> + /*
> + * For any number of values inserted, separator at the end needs to be
> + * replaced by string terminator
> + */
> + if (str.len > 1)
> + {
> + str.data[str.len - 1] = '\0';
> + str.len -= 1;
> + appendStringInfoString(&str, "}");
> + values[1] = str.data;
> + }
> + else
> + values[1] = NULL;

I don't like this at all. I would write it something like this:

n = 0;

/* hba->conntype stuff */

n++;
if (list_length(hba->databases) != 0)
{
initStringInfo(&str);
appendStringInfoString(&str, "{");

foreach(cell, hba->databases)
{
tok = lfirst(cell);
appendStringInfoString(&str, tok->string);
appendStringInfoString(&str, ",");
}

str.data[str.len - 1] = '}';
values[n] = str.data;
}
else
values[n] = NULL;

Note the variable n instead of using 0/1/… indexes into values, and that
I moved the call to initStringInfo from the beginning of the function.

The same applies to the other cases below.

(But this is, of course, all subject to whatever solution is found to
the all/{all} problem.)

> /* Retrieving Socket address */
> switch (hba->addr.ss_family)
> {
> case AF_INET:
> len = sizeof(struct sockaddr_in);
> break;
> #ifdef HAVE_IPV6
> case AF_INET6:
> len = sizeof(struct sockaddr_in6);
> break;
> #endif
> default:
> len = sizeof(struct sockaddr_storage);
> break;
> }
> if (getnameinfo((const struct sockaddr *) & hba->addr, len, buffer, sizeof(buffer), NULL, 0, NI_NUMERICHOST) == 0)
> values[3] = pstrdup(buffer);
> else
> values[3] = NULL;

This should use pg_getnameinfo_all. You don't need the switch, just pass
in .salen. Use a buffer of NI_MAXHOST, not 256. Look for other calls to
pg_getnameinfo_all for examples. (Also split long lines.)

(Note: this pstrdup is OK.)

> /* Retrieving socket mask */
> switch (hba->mask.ss_family)
> {
> case AF_INET:

Same.

> + case ipCmpMask:
> + values[5] = pstrdup("Mask");
> + break;

Lowercase, no pstrdup.

> + case uaReject:
> + values[7] = pstrdup("Reject");
> + break;

Same.

> + if ((hba->usermap) && (hba->auth_method == uaIdent || hba->auth_method == uaPeer || hba->auth_method == uaCert || hba->auth_method == uaGSS || hba->auth_method == uaSSPI))

Split across lines.

> + appendStringInfoString(&str, pstrdup(hba->ldapserver));

No need for the many, many pstrdup()s.

> + if (str.len > 1)
> + {
> + str.data[str.len - 1] = '\0';
> + str.len -= 1;
> + appendStringInfoString(&str, "}");
> + values[8] = str.data;
> + }
> + else
> + values[8] = NULL;

This should become:

if (str.len != 0)
{
str.data[str.len - 1] = '}';
values[n] = str.data;
}
else
values[n] = NULL;

> + /* Retrieving linenumber */
> + snprintf(buffer, sizeof(buffer), "%d", hba->linenumber);
> + values[9] = pstrdup(buffer);

Use psprintf() and get rid of the static buffer.

> + /* stuff done only on the first call of the function */
> + if (SRF_IS_FIRSTCALL())
> + {
> + /* create a function context for cross-call persistence */
> + funcctx = SRF_FIRSTCALL_INIT();

This is not a comment about your patch, just a general complaint about
how everyone cargo-cults ALL of these comments for EVERY SINGLE SRF.
Isn't "if (SRF_IS_FIRSTCALL())" _perfectly_ clear, or is it just me?

I'd just as soon rip them all out of this patch.

> + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "connection_type",
> + TEXTOID, -1, 0);

(See comments below about column names.)

> + else
> + {
> + /* do when there is no more left */
> + SRF_RETURN_DONE(funcctx);
> + }

I'd just say SRF_RETURN_DONE(funcctx) at the end without the else block.

> + DATA(insert OID = 3206 ( pg_show_all_hba_settings PGNSP PGUID 12 1 1000 0 0 f f f f t t s 0 0 2249 "" "{25,1009,1009,25,25,25,25,25,1009,23}" "{o,o,o,o,o,o,o,o,o,o}" "{connection_type,databases,roles,socket_address,socket_mask,compare_method,hostname,authmethod,configuration_options,linenumber}" _null_ show_all_hba_settings _null_ _null_ _null_ ));
> + DESCR("SHOW ALL HBA settings as a function");

OID 3206 is now in use by the #>> operator, so you'll have to pick
another when you resubmit.

I went through the values one by one, and they look OK. But see comments
below about the column names.

The description should be "Return client authentication settings" or so.

> + <para>
> + The view <structname>pg_hba_settings</structname> provides access to
> + useful information about authentication details of Postgresql cluster.
> + For security reasons, access to this view is limited only to superusers. No update, insert or delete operations are allowed in this view.
> + </para>

I suggest:

The read-only <structname>pg_hba_settings</structname> view provides
access to the client authentication configuration from pg_hba.conf.
Access to this view is limited to superusers.

(So this addresses Jaime's comment: the view is already superuser-only.)

> + <entry><structfield>connection_type</structfield></entry>

I'd call this just "type" to match pg_hba.conf.

> + <entry><structfield>databases</structfield></entry>
> + <entry><type>text[]</type></entry>
> + <entry>List of database names</entry>

I'm really not a fan of returning an array with e.g. "{all}". But I'm
not sure what to do instead. I think the really right thing to do would
be to have two separate columns, one with "all", "sameuser", "samerole",
"replication", or empty; and the other an array of database names.

I'd like to hear what others think about this.

> + <entry><structfield>roles</structfield></entry>

Same problem.

> + <entry><structfield>socket_address</structfield></entry>

Just "address".

> + <entry><structfield>socket_mask</structfield></entry>

Just "mask".

> + <entry><structfield>authmethod</structfield></entry>

Just "method".

> + <entry><structfield>configuration_options</structfield></entry>

Just "options".

> + <entry><structfield>linenumber</structfield></entry>

Should be "line_number", I think.

> + <para>
> + Details of this file can be accessed from pg_hba_settings view.
> + See <xref linkend="view-pg-hba-settings"> for details.
> + </para>

I suggest "The contents of this file are reflected in the
pg_hba_settings view. See … for details."

Do you have the time to rework the patch along these lines and resubmit
for this commitfest (i.e. in the next few days, certainly sometime this
week)? If not, please let me know and I will move it to the next CF to
give you time to make the changes.

Thank you for your contribution. Although I've asked for a lot of
changes, I think the feature is certainly one we want.

-- Abhijit