Re: pg_hba_file_settings view patch

On Wed, Jan 25, 2017 at 9:58 AM, Haribabu Kommi
<kommi(dot)haribabu(at)gmail(dot)com> wrote:
>
>
> On Wed, Jan 25, 2017 at 2:50 PM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>>
>> On Wed, Jan 25, 2017 at 6:34 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > On Tue, Jan 24, 2017 at 11:19 PM, Ashutosh Bapat
>> > <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>> >> On Mon, Jan 23, 2017 at 1:43 PM, Haribabu Kommi
>> >> <kommi(dot)haribabu(at)gmail(dot)com> wrote:
>> >>>
>> >>>
>> >>> On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> >>>>
>> >>>> Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> writes:
>> >>>> > [ pg_hba_rules_10.patch ]
>> >>>>
>> >>>> I took a quick look over this.
>> >>>
>> >>>
>> >>> Thanks for the review.
>> >>>
>> >>>>
>> >>>> * I'm not exactly convinced that the way you approached the error
>> >>>> message
>> >>>> reporting, ie duplicating the logged message, is good. In particular
>> >>>> this results in localizing the strings reported in
>> >>>> pg_hba_rules.error,
>> >>>> which is exactly opposite to the decision we reached for the
>> >>>> pg_file_settings view. What's the reasoning for deciding that this
>> >>>> view should contain localized strings? (More generally, we found in
>> >>>> the pg_file_settings view that we didn't always want to use exactly
>> >>>> the same string that was logged, anyway.)
>> >>>
>> >>>
>> >>> Actually there is no particular reason to display the localized
>> >>> strings,
>> >>> Just thought that it may be useful to the user if it get displayed in
>> >>> their
>> >>> own language. And also doing this way will reduce the error message
>> >>> duplicate in the code that is used for display in the view and writing
>> >>> it
>> >>> into the log file.
>> >>>
>> >>
>> >> Would it be better, if we could parse each HBA line within
>> >> PG_TRY()/PG_CATCH() and read errmsg from errordata stack in
>> >> PG_CATCH()? We do that only when errcode is ERRCODE_CONFIG_FILE_ERROR,
>> >> PG_THROWing otherwise. That's probably a bad idea but wanted to put it
>> >> out as it came to me. It would eliminate a lot of changes in this
>> >> patch.
>> >
>> > It still needs to save the error message string somewhere. So I am not
>> > sure that it would save much patch size.
>>
>> My understanding is that ereport (and some other calls included in
>> that statement) call saves it on errordata stack before jumping to the
>> handler.
>
>
> 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.

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