Re: pg_hba_file_settings view patch

On Thu, Jan 26, 2017 at 4:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> writes:
> > On Wed, Jan 25, 2017 at 9:58 AM, Haribabu Kommi
> > <kommi(dot)haribabu(at)gmail(dot)com> wrote:
> >> All the ereport messages of level are LOG, because of this reason,
> because
> >> of this reason even if we use the TRY/CATCH, it doesn't work. As the
> >> messages gets printed to the logfile and continue to process the next
> >> statement.
>
> > Right. Sorry for missing to mention about this change in the patch.
> > Originally the messages are at level ERROR so TRY/CATCH will be able
> > to catch it. We will need to somehow then turn ERROR to LOG and
> > re-throw it. I haven't tried it myself though.
>
> I do not think throwing/catching errors is a good idea here. It will mean
> that the view can't report more than one mistake per run, and it will
> create a significant difference in the parsing code's control flow between
> "normal" and "read for view" modes, which is a recipe for bugs. Also,
> it's different from the way things are done for the pg_file_settings view.
> For the sake of future developers, I think we should make this work as
> much like that view as we can.
>
> The way I'd be inclined to make the individual reporting changes is like
>
> if (!EnableSSL)
> + {
> - ereport(LOG,
> + ereport(elevel,
> (errcode(ERRCODE_CONFIG_FILE_ERROR),
> errmsg("hostssl record cannot match because SSL is
> disabled"),
> errhint("Set ssl = on in postgresql.conf."),
> errcontext("line %d of configuration file
> \"%s\"",
> line_num, HbaFileName)));
> + *err_msg = pstrdup("hostssl record cannot match because
> SSL is disabled");
> + }
>
> which is considerably less invasive and hence easier to review, and
> supports reporting different text in the view than appears in the log,
> should we need that. It seems likely also that we could drop the pstrdup
> in the case of constant strings (we'd still need psprintf if we want to
> insert values into the view messages), which would make this way cheaper
> than what's in the patch now.
>

Updated patch attached as per the above modifications.

This patch currently doesn't have the code for reporting the two log
messages
that can occur in tokenize_file function. To support the same, I am
thinking of
changing line_nums list to line_info list that can contain both line number
and
the error message that occurred during the tokenize. This list data is used
to identify whether that line is having any error or not before parsing
that hba
line, and directly report that line as error in the view.

Any comments/suggestions in proceeding with that implementation.

Regards,
Hari Babu
Fujitsu Australia