On Wed, Dec 9, 2015 at 6:48 AM, Haribabu Kommi <kommi.harib...@gmail.com>
wrote:
>
> On Wed, Dec 9, 2015 at 2:36 AM, Amit Kapila <amit.kapil...@gmail.com>
wrote:
> >
> > I think it is better to have check on number of args in the
> > above functions similar to what we have in ginarrayextract_2args.
>
> ginarrayextract_2args is an deprecated function that checks and returns
> error if user is using with two arguments. But in pg_hba_lookup function,
> providing two argument is a valid scenario. The check can be added only
> to verify whether the provided number of arguments are two or not. Is it
> really required?
>
I think we can live without such a check especially because you
already have required check for function args in pg_hba_lookup
function.
> > 2.
> > +
> > + /*
> > + * Reload authentication config files too to refresh
> > + * pg_hba_conf view data.
> > + */
> > + if (!load_hba())
> > + {
> > + ereport(DEBUG1,
> > + (errmsg("Falure in reloading pg_hba.conf, pg_hba_conf view may show
stale
> > information")));
> > + load_hba_failure = true;
> > + }
> > +
> > + load_hba_failure = false;
> >
> > Won't the above code set load_hba_failure as false even in
> > failure case.
>
> Fixed.
>
Another bigger issue I see in the above part of code is that it doesn't
seem to be safe to call load_hba() at that place in PostgresMain() as
currently load_hba() is using a context created from PostmasterContext
to perform the parsing and some other stuff, the PostmasterContext
won't be available at that time. It is deleted immediately after
InitPostgres
is completed. So either we need to make PostmasterContext don't go
away after InitPostgres() or load_hba shouldn't use it and rather use
CurrentMemroyContext similar to ProcessConfigFile or may be use
TopMemoryContext instead of PostmasterContext if possible. I think
this needs some more thoughts.
Apart from above, this patch doesn't seem to work on Windows or
for EXEC_BACKEND builds as we are loading the hba file in a
temporary context (PostmasterContext, refer PerformAuthentication)
which won't be alive for the backends life. This works on linux because
of fork/exec mechanism which allows to inherit the parsed file
(parsed_hba_lines). This is slightly tricky problem to solve and we
have couple of options (a) use TopMemoryContext instead of Postmaster
Context to load hba; (b) Use CurrentMemoryContext (c) pass the parsed
hba file for Windows/Exec_Backend using save_backend_variables/
restore_backend_variables mechanism or if you have any other idea.
If you don't have any better idea, then you can evaluate above ideas
and see which one makes more sense.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
