Thread: pg_hba.conf - patch to report all parsing errors, and then bail

Currently, load_hba() bails on the first parsing error. It would be
better for the typo-prone sysadmin if it reported ALL errors, and THEN
bailed out.

This patch implements that behavior.  Tested against 8.4 HEAD this morning.

Idea is to do a similar thing for postgresql.conf. That is a little more
complicated and will be a separate patch.

-selena


--
Selena Deckelmann
End Point Corporation
selena@endpoint.com
503-282-2512
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 6923d06..c6a7ba7 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1304,6 +1304,7 @@ load_hba(void)
     List *hba_line_nums = NIL;
     ListCell   *line, *line_num;
     List *new_parsed_lines = NIL;
+    bool OK = true;

     file = AllocateFile(HbaFileName, "r");
     if (file == NULL)
@@ -1332,17 +1333,22 @@ load_hba(void)

         if (!parse_hba_line(lfirst(line), lfirst_int(line_num), newline))
         {
-            /* Parse error in the file, so bail out */
+            /* Parse error in the file, so indicate there's a problem */
             free_hba_record(newline);
             pfree(newline);
             clean_hba_list(new_parsed_lines);
             /* Error has already been reported in the parsing function */
-            return false;
+            OK = false;
+            continue;
         }

         new_parsed_lines = lappend(new_parsed_lines, newline);
     }

+    if (!OK)
+        /* Parsing failed, so bail out */
+        return false;
+
     /* Loaded new file successfully, replace the one we use */
     clean_hba_list(parsed_hba_lines);
     parsed_hba_lines = new_parsed_lines;

Selena Deckelmann <selena@endpoint.com> writes:
> Currently, load_hba() bails on the first parsing error. It would be 
> better for the typo-prone sysadmin if it reported ALL errors, and THEN 
> bailed out.

> This patch implements that behavior.  Tested against 8.4 HEAD this morning.

It sure looks like that's going to try to free new_parsed_lines more
than once.  Shouldn't clean_hba_list be done just once?
        regards, tom lane

Tom Lane wrote:
> Selena Deckelmann <selena@endpoint.com> writes:
>> Currently, load_hba() bails on the first parsing error. It would be 
>> better for the typo-prone sysadmin if it reported ALL errors, and THEN 
>> bailed out.
> 
>> This patch implements that behavior.  Tested against 8.4 HEAD this morning.
> 
> It sure looks like that's going to try to free new_parsed_lines more
> than once.  Shouldn't clean_hba_list be done just once?

Yeah, it should be done in the if branch down below. And I think by our
coding standards (or at least by our conventions), the variable should
be "ok" and not "OK".

Unless there are any objections, I can do those cleanups and apply it.

//Magnus

Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> It sure looks like that's going to try to free new_parsed_lines more
>> than once.  Shouldn't clean_hba_list be done just once?

> Yeah, it should be done in the if branch down below. And I think by our
> coding standards (or at least by our conventions), the variable should
> be "ok" and not "OK".

> Unless there are any objections, I can do those cleanups and apply it.

I thought the adjacent comments could do with a bit more wordsmithing,
also, but otherwise that about covers it.
        regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> Tom Lane wrote:
>>> It sure looks like that's going to try to free new_parsed_lines more
>>> than once.  Shouldn't clean_hba_list be done just once?
> 
>> Yeah, it should be done in the if branch down below. And I think by our
>> coding standards (or at least by our conventions), the variable should
>> be "ok" and not "OK".
> 
>> Unless there are any objections, I can do those cleanups and apply it.
> 
> I thought the adjacent comments could do with a bit more wordsmithing,
> also, but otherwise that about covers it.

Applied.

//Magnus

Magnus Hagander wrote:

>
> Applied.
>
> //Magnus

Thanks.

And thanks, Tom, for pointing out my multiple attempts to free.

-selena

-- 
Selena Deckelmann
End Point Corporation
selena@endpoint.com
503-282-2512