Robert Haas <robertmh...@gmail.com> writes:
> On Fri, Apr 8, 2016 at 3:24 PM, David Steele <da...@pgmasters.net> wrote:
>> Can one of the reviewers decide if this is ready to commit? I fear it
>> will be pushed to the next CF otherwise. I don't think the committers
>> have time to make that determination today...
> Well, it's not getting committed unless some committer determines that
> it is ready to commit.
I took a quick look; IMO it is certainly not ready to commit.
* Why is the IP address parameter declared as "text" and not "inet"?
Why is it optional --- it doesn't seem to me that it can have a useful
default value?
* Docs claim that ssl_inuse is of type text, when it's really bool,
and that the result is record when it's really setof record.
* While I agree that allowing ssl_inuse to default to false is probably
okay, I wonder how well this function signature will cope if we ever add
more things that pg_hba matching depends on.
* The patch seems mighty invasive to the auth code, which is not really
a place where we want a lot of churn and opportunity for mistakes. Is
there another way to do this? Do we really *need* a line-by-line report
of why specific lines didn't match?
* I'm a tad suspicious of the memory management, in particular the
random-looking rearrangement of where PostmasterContext gets created
and deleted. It'd likely be better to fix things so that load_hba
doesn't have a hard-wired reference to PostmasterContext in the first
place, but is told which context to allocate under.
More generally, I'm not convinced about the use-case for this patch.
What problem is it supposed to help in dealing with, exactly? Not syntax
errors in the hba file, evidently, since it doesn't make any attempt to
instrument the file parser. And it's not very clear that it'll help
with "I can't connect", either, because if you can't connect you're
not going to be running this function.
If people actually need more help in figuring out why the hba line matcher
selected the line it did, I'm inclined to think maybe what's needed is a
logging option that would print a verbose trace of match/no-match
decisions. More or less the same data this returns, really, but directed
to the postmaster log. That way you'd be able to see it even when you're
not getting let into the database.
On the whole, though, I wonder if we shouldn't just tweak log_connections
so that it records which pg_hba line was matched. (We already have
logging about which line was matched on an auth failure.) I'm not really
convinced that a SQL function like this is going to be very helpful.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
