Re: pg_hba_file_settings view patch

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.

>> * getauthmethod() might be better replaced with an array. And doesn't it
>> produce uninitialized-variable warnings for you?
>
>
> No, i am not getting any warnings.
> Changed to a static array.

Thanks. Probably we should update parse_hba_line() to keep it in sync
with the array. But that may be a separate add-on patch.

Rest of the patch looks good to me.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company