On Wed, Dec 9, 2015 at 2:36 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > Few assorted comments:
Thanks for the review. > 1. > + /* > + * SQL-accessible SRF to return all the settings from the pg_hba.conf > + * file. > + */ > + Datum > + pg_hba_lookup_2args(PG_FUNCTION_ARGS) > + { > + return pg_hba_lookup(fcinfo); > + } > + > + /* > + * SQL-accessible SRF to return all the settings from the pg_hba.conf > + * file. > + */ > + Datum > + pg_hba_lookup_3args(PG_FUNCTION_ARGS) > + { > + return pg_hba_lookup(fcinfo); > + } > > 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? > 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. > 3. > + Datum > + pg_hba_lookup(PG_FUNCTION_ARGS) > + { > + char *user; > + char *database; > + char *address; > + char *hostname; > + bool ssl_inuse = false; > + struct sockaddr_storage addr; > + hba_lookup_args_mode args_mode = TWO_ARGS_MODE; /* Minimum number of > arguments */ > + > + /* > + * We must use the Materialize mode to be safe against HBA file reloads > + * while the cursor is open. It's also more efficient than having to look > + * up our current position in the parsed list every time. > + */ > + ReturnSetInfo *rsi = (ReturnSetInfo *)fcinfo->resultinfo; > + > + if (!superuser()) > + ereport(ERROR, > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > + (errmsg("only superuser can view pg_hba.conf settings")))); > > To be consistent with other similar messages, it is better to > start this message with "must be superuser ..", refer other > similar superuser checks in xlogfuncs.c Updated as "must be superuser to view". Attached updated patch after taking care of review comments. Regards, Hari Babu Fujitsu Australia
pg_hba_lookup_poc_v6.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers