Re: pg_hba_lookup function to get all matching pg_hba.conf entries

Hi,

I've reviewed the patch today, after re-reading the whole discussion.

Overall I'm quite happy with the design - a function is certainly better
for the use-case. Not just because of the keyword handling issues, but
because the checks happen in a particular order and terminate once a
match is found, and that's very difficult to do with a view. So +1 to
the function approach. Also +1 to abandoning the NOTICE idea and just
generating rows.

The one unsolved issue is what to do with 1e24cf64. My understanding is
that the current patch still requires reverting of that patch, but my
feeling is TL won't be particularly keen about doing that. Or am I
missing something?

The current patch (v6) also triggers a few warnings during compilation,
about hostname/address being unitialized in pg_hba_lookup(). That
happens because 'address' is only set when (! PG_ARGISNULL(2)). Fixing
it is as simple as

char *address = NULL;
char *hostname = NULL;

at the beginning of the function (this seems correct to me).

The current patch also does not handle 'all' keywords correctly - it
apparently just compares the values as strings, which is incorrect. For
example this

SELECT * FROM pg_hba_lookup('all', 'all')

matches this pg_hba.conf line

local all all trust

That's clearly incorrect, as Alvaro pointed out.

I'm also wondering whether we really need three separate functions in
pg_proc.

pg_hba_lookup(database, user)
pg_hba_lookup(database, user, address)
pg_hba_lookup(database, user, address, ssl_inuse)

Clearly, that's designed to match the local/host/hostssl/hostnossl cases
available in pg_hba. But why not to simply use default values instead?

pg_hba_lookup(database TEXT, user TEXT,
address TEXT DEFAULT NULL,
ssl_inuse BOOLEAN DEFAULT NULL)

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services