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
Attachments:
hba_report_all_parse_errors_v1.patchtext/plain; name=hba_report_all_parse_errors_v1.patch; x-mac-creator=0; x-mac-type=0Download
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