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)


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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to