Re: pg_hba_file_settings view patch

On Wed, Nov 16, 2016 at 4:40 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> make check run with this patch shows server crashes. regression.out
> attached. I have run make check after a clean build, tried building it
> after running configure, but the problem is always reproducible. Do
> you see this problem?

> Also the patch has a white space error.
> git diff --check
> src/backend/utils/init/postinit.c:729: space before tab in indent.
> +       /*
>

I looked at the patch in some more details and here are some more comments
1. In catalog.sgml, the sentence "If the configuration file contains any errors
..." looks redundant, as description of "error" field says so. Removed it in
the attached patch. In that example, you might want to provide pg_hba.conf
contents to help understand the view output.

2. I think the view will be useful, if loading the file did not have the
desired effects, whether because of SIGHUP or a fresh start. So, may be the
sentence "for diagnosing problems if a SIGHUP signal did not have the desired
effects.", should be changed to be more generic e.g. ... if loading file did
not have ... .

3. Something wrong with the indentation, at least not how pg_indent would indent
the variable names.
+typedef struct LookupHbaLineCxt
+{
+    MemoryContext memcxt;
+    TupleDesc    tupdesc;
+    Tuplestorestate *tuple_store;
+} LookupHbaLineCxt;

+static void lookup_hba_line_callback(void *context, int lineno,
HbaLine *hba, const char *err_msg);
Overflows 80 character limit.

in parse_hba_line()
-        ereport(LOG,
+        ereport(level,
                 (errcode(ERRCODE_CONFIG_FILE_ERROR),
                  errmsg("invalid connection type \"%s\"",
                         token->string),
                  errcontext("line %d of configuration file \"%s\"",
                             line_num, HbaFileName)));
+        *err_msg = pstrdup(_("invalid connection type"));

Is the difference between error reported to error log and that in the view
intentional? That brings to another question. Everywhere, in similar code, the
patch adds a line *err_msg = pstrdup() or psprinf() and copies the arguements
to errmsg(). Someone modifying the error message has to duplicate the changes.
Since the code is just few lines away, it may not be hard to duplicate the
changes, but still that's a maintenance burder. Isn't there a way to compute
the message once and use it twice? show_all_file_settings() used for
pg_file_settings also has similar problem, so may be it's an accepted practice.
There are multiple instances of such a difference, but may be the invalid value
can be found out from the value of the referenced field (which will be part of
the view). So, may be it's ok. But that not true with the difference below.
gai_strerror() may not be obvious from the referenced field.
-                ereport(LOG,
+                ereport(level,
                         (errcode(ERRCODE_CONFIG_FILE_ERROR),
                          errmsg("invalid IP address \"%s\": %s",
                                 str, gai_strerror(ret)),
                          errcontext("line %d of configuration file \"%s\"",
                                     line_num, HbaFileName)));
                 if (gai_result)
                     pg_freeaddrinfo_all(hints.ai_family, gai_result);
+                *err_msg = pstrdup(_("invalid IP address"));

4.
+    if (!rsi || !IsA(rsi, ReturnSetInfo) ||
+        (rsi->allowedModes & SFRM_Materialize) == 0)
+        ereport(ERROR,
+                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                 errmsg("set-valued function called in context that
cannot accept a set")));
show_all_file_settings(), a function similar to this one, splits the condition
above into two and throws different error message for each of them.
    /* Check to see if caller supports us returning a tuplestore */
    if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
        ereport(ERROR,
                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                 errmsg("set-valued function called in context that
cannot accept a set")));
    if (!(rsinfo->allowedModes & SFRM_Materialize))
        ereport(ERROR,
                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                 errmsg("materialize mode required, but it is not " \
                        "allowed in this context")));
Why is this difference?

5. May be you want to rename "type" attribute to "connection_type" to be
explicit.

6. The attribute names "keyword_database" and "keyword_user" do not seem to be
appropriate. They do not look like keywords as such. They are more like
synonyms or collection (value replication is an exception). May be you want to
rename those as "database_keyword" or "user_keyword" similar to the naming
convention of token_is_a_database_keyword(). I agree that the usage
can not be described in a single phrase correctly, and pg_hba.conf
documentation too doesn't help much. Similarly for keyword_address.

7. Also, each of the fields, database, user, address can contain multiple
values in pg_hba.conf. So may be corresponding attributes should be named as
plural rather than singular.

8. If any of the parsed lines has an error parse_hba_line() returns a NULL
line. In order to avoid memory leak, load_hba() runs this function in a memory
context, which is destroyed when an error is encountered. This also destroys
any previous lines which were parsed correctly. IIUC, the patch uses the
callback to save the contents of those lines in a different context, so that an
error wouldn't destroy those. But using a callback doesn't seem like a good way
to do this. Furthermore the code assumes that if callback is provided the error
level is always DEBUG3 or the caller doesn't want to update the saved
authentication rules etc. If in future someone wants to add another callback
function but doesn't want error level to be DEBUG3 or still wants to update the
saved rules, we will need conditional statements based on the value of
callback. That doesn't seems to be something which should be done with
callbacks. I don't think that's flexible. A better design may be to let
load_hba() accept errorlevel, and flag indicating whether to ignore errors as
an argument and return a list of parsed lines. If there is an error and the
flag indicates not to ignore the error, we destroy the memory context and
return NIL. The list can be then used to update the saved hba rules or to
process further (e.g. to feed the function hba_rules()). hbacxt also can an
OUTPUT arguemnt to the function or an argument passed in by the caller.

9. I am not able to understand, why does this patch need changes to
load_ident(). Sorry, if I have missed any previous discussion on this topic.