Re: pg_hba_file_settings view patch

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.

regards, tom lane