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

On Wed, Dec 16, 2015 at 8:19 AM, Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> I've reviewed the patch today, after re-reading the whole discussion.

Thanks for the review.

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

Until pg_hba_lookup function, the parsed hba lines are not used in the backend.
These are used only postmaster process for the authentication. As the parsed
hba lines occupy extra memory in the backend process which is of no use.
Because of this reason TL has changed it to PostmasterContext instead of
TopMemoryContext.

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

corrected.

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

In the above case, the 'all' is taken as a database and user names.
The pg_hba line contains the keyword of 'all' as database and user.
This line can match with any database and user names provided
by the user. Because of this reason, it matches with the first line
of pg_hba.conf.

I feel it is fine. Please let me know if you are expecting a different
behavior.

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

Function is changed to accept default values.

Apart from the above, added a local memory context to allocate the memory
required for forming tuple for each line. This context resets for every hba line
to avoid consuming unnecessary memory for scenarios of huge pg_hba.conf
files.

In the revert_hba_context_release_in_backend patch, apart from reverting
the commit - 1e24cf64. In tokenize_file function, changed the new context
allocation from CurrentMemoryContext instead of TopMemoryContext.

Patch apply process:
1. revert_hba_context_release_in_backend_2.patch
2. pg_hba_lookup_poc_v7.patch

Regards,
Hari Babu
Fujitsu Australia