Re: pg_hba_file_settings view patch

On Thu, Nov 17, 2016 at 10:13 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> On Wed, Nov 16, 2016 at 4:40 PM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)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?
>

Thanks for reviewing the patch.

No. I am not able to reproduce this problem.
make check works fine in my system.

From the regression.out file, the crash occurred in select_parallel.out,
I don't think this patch has any affect on that test.

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

corrected.

> 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.
>

updated details, but not exactly what you said. check it once.

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 ... .
>

changed.

> 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;
>

corrected.

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

corrected.

> 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"));
>

Reused the error string once, as in this patch it chances in many places
compared
to pg_file_settings, so I tend to reuse it.

> 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?
>

changed according to show_all_file_settings() function.

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

"type" is the keyword that is mentioned in the pg_hba.conf, I feel it is
better
if this view is in sync with that. If others feel the same, I can change.

> 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.
>

Changed.

> 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.
>

Same answer as to the question - 5.

> 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.
>

hba_rules() function cannot operate on final parsed hba lines, because it
has
to store the error that is present in the line, that can be obtained only
during
the parse stage.

The hba rules are needed only for the authentication purpose and those
shouldn't
be stored in the individual backend. Because of this reason after every
operation
the parsed rules are cleared.

Added a flag to pass the log level.

> 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.

Earlier, the pg_hba.conf is loaded into the Postmastercontext, this is
causing
problems for this patch. So In the patch it is changed into
currentmemorycontext
and deleted the data at the end. The similar change is carried out for ident
functionality, because of this reason, it is shown in this patch. May be I
can
separate that into an individual patch.

Updated patch is attached.

Regards,
Hari Babu
Fujitsu Australia