Re: [HACKERS] pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> [ pg_hba_rules_10.patch ]

I took a quick look over this.

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

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

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

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

* getauthmethod() might be better replaced with an array.  And doesn't it
produce uninitialized-variable warnings for you?

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

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

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

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

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

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

* 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