Re: pg_hba_file_settings view patch

On Fri, Nov 18, 2016 at 12:23 PM, Haribabu Kommi
<kommi(dot)haribabu(at)gmail(dot)com> wrote:
>
>
> 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.

It could be because of some un-initialised variable, which is
initialized appropriately by default on your machine but not on my
machine. I first applied your pg_hba_rules... patch, ran regression.
It didn't crash. then I applied patch for discard_hba... and it
started crashing. Does that give you any clue? Here's regression.out
file for make installcheck. Here is error log snippet that shows a
SIGSEGV there.
2016-11-22 15:47:11.939 IST [86206] LOG: worker process: parallel
worker for PID 86779 (PID 86780) was terminated by signal 11:
Segmentation fault
2016-11-22 15:47:11.939 IST [86206] LOG: terminating any other active
server processes
2016-11-22 15:47:11.939 IST [86779] WARNING: terminating connection
because of crash of another server process
2016-11-22 15:47:11.939 IST [86779] DETAIL: The postmaster has
commanded this server process to roll back the current transaction and
exit, because another server process exited abnormally and possibly
corrupted shared memory.

Applying those patches in any order doesn't matter.

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

The changes in postinit.c may have that impact. Just a guess though. I
haven't debugged the crash myself.

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

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

Thanks. Although the new change might affect the way we translate the
messages in other languages. I am not sure. So, I will leave it for
someone with more knowledge to review.

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

Ok.

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

Ok.

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

/*
+ * If callback function is available, then don't update the saved
+ * authentication rules.
+ */
+ if (callback)
+ {
+ MemoryContextDelete(linecxt);
+ MemoryContextSwitchTo(oldcxt);
+ MemoryContextDelete(hbacxt);
+ return true;
+ }

this still remains problematic, in case another user of load_hba wants
to pass a callback but wants to update the saved rules. Usually,
callbacks are used when the decision to modify certain logic is far
away in time and code from the actual place where the changes are to
be applied, e.g. FDW callbacks. But here we that's not the case. Is
there any precedence in code for something like this.

What we may want to do, is separate the logic of reading the hba rules
in a list and the logic to update existing rules in two different
functions e.g read_hba() and load_hba(). hba_rules() can use
read_hba() with ignore errors flag to get the list of hba lines. It
can then use this list to create tuples to be returned in hba_rules().
load_hba() will read_hba() with memory reset on error flag (same
boolean) to read the list of hba lines and update the saved rules if
there's no error. err_msg can be either a field in HbaLine, which will
be used only by hba_rules() OR read_hba() could return list of
err_msgs as a pass by ref argument.

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

I think we need to include the hba related changes in this patch and
indent related changes should be moved to the other patch.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company