Re: pg_hba_file_settings view patch

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.

> * Also, there seems to be a lot of ereports remaining unconverted,
> eg the "authentication file token too long" error. One of the things
> we wanted pg_file_settings to be able to do was finger pretty much any
> mistake in the config file, including syntax errors. It seems like
> it'd be a shame if pg_hba_rules is unable to help with that. You
> should be able to fill in line number and error even if the line is
> too mangled to be able to populate the other fields sanely.
>

The two errors that are missed are, "could not open secondary
authentication file"
and "authentication file token too long" errors. For these two cases, the
server
is not throwing any error, it just logs the message and continues. Is it
fine to add
these these two cases as errors in the view?

> * While we're on the comparison to pg_file_settings ... pg_hba_rules
> is not the view name I'd guess if I guessed one based on that precedent.
> I don't have a better suggestion offhand, but this name seems weirdly
> inconsistent.
>

People are suggested to use "rules" instead of "settings", as the entries
in the pg_hba.conf are used as rules to control the client authentication
mechanism.

> * I think "memcxt" as a field name is pretty unhelpful if you suppose
> it just means "memory context", and outright misleading if you guess
> it's, say, the context the tuplestore is in. Perhaps call it "tmpcxt"
> and add a comment like "Short-lived context, reset after each line".
> The other fields of FillHbaLineCxt could do with comments too.
>
> * ... although really, you've gone way overboard with temp contexts
> here. I don't think it's necessary to have a per-line context at all;
> you could just do all the work in the single temp context that fill_hba
> calls hbacxt, and drop it all at end of function, because no matter what
> you'll be eating O(file size) space, and we're just quibbling over the
> size of the multiplier. Also, if you're concerned with reclaiming space
> before end of query, aren't you leaking the tokenize_file output data?
>

Removed the temp context and done everything in a single context.

>
> * 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.

>
> * It seems a little weird that fill_hba_auth_opt isn't inserting the "="
> between name and value. And should it be using psprintf? It's the
> only use of StringInfo in this file, so it looks a bit out of place.
> Actually, I wonder if you wouldn't be better off replacing it with a
> coding style like
>
> options[noptions++] =
> CStringGetTextDatum(psprintf("ldapport=%d", hba->ldapport));
>
> which seems more readable and more flexible.
>

Corrected accordingly.

>
> * "MAX_OPTIONS" is, uh, mighty generic. Maybe "MAX_HBA_OPTIONS"?
> And the comment for it doesn't actually tell you what it is.
>

Updated.

>
> * NUM_PG_HBA_LOOKUP_ATTS seems like it ought to match the name of the
> view, ie NUM_PG_HBA_RULES_ATTS if that doesn't get renamed.
>

Updated to the current name.

> * Usually we just write "if (listvar)" or "if (listvar != NIL)" rather
> than "if (list_length(listvar) != 0)". list_length() overspecifies what
> you need to test. This isn't as critical as it was back in the day when
> list_length() cost O(N), but still the former is much more common project
> style.
>

Corrected.

* Why is AllocateFile() failure only an ereport(LOG) in fill_hba()?
> From the user's viewpoint he'll get an empty view with no visible
> reason. Probably ereport(ERROR) is more sensible. You could imagine
> trying to show the error in the view, but that seems like more work
> than the case warrants.
>

Corrected.

> * Seems like the FillHbaLineCxt variable could just be a local struct
> in hba_rules(), and dispense with one palloc/pfree cycle.
>

Removed the FillHbaLineCxt structure itself, after removing the
memory context variable, it just have two variables, directly passed
them as an arguments.

> * I'm not really on board with patches modifying pgindent/typedefs.list
> retail. To my mind that file represents the typedefs used the last
> time we pgindent'd the whole tree, and if you want an up-to-date list
> you should ask the buildfarm. Otherwise there's just too much confusion
> stemming from the fact that not everybody updates it when patching.
>
> My own workflow for reindenting patches goes more like
> curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
> my-typedefs.list
> ... manually edit my-typedefs.list to add any new typedefs from patch ...
> pgindent --typedefs=my-typedefs.list target-files
>

Ok. Thanks for the information. I followed the above steps for the
indentation.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia