On Mon, Apr 06, 2020 at 10:12:16PM +0000, Cary Huang wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: tested, passed
> Documentation: tested, passed
>
> Hi
> I applied the patch "v3-0001-Enable-setting-pg_hba.conf-permissions-from-initd.patch" and did some verification with
it.The intended feature works overall and I think it is quite useful to support default auth methods for ssl and gss
hosttypes. I have also found some minor things in the patch and would like to share as below:
>
> > +"# CAUTION: Configuring the system for \"trust\" authentication\n" \
> > +"# allows any user who can reach the databse on the route specified\n" \
> > +"# to connect as any PostgreSQL user, including the database\n" \
> > +"# superuser. If you do not trust all the users who could\n" \
> > +"# reach the database on the route specified, use a more restrictive\n" \
> > +"# authentication method.\n"
>
> Found a typo: should be 'database' instead of 'databse'
Fixed.
> > * a sort of poor man's grep -v
> > */
> > -#ifndef HAVE_UNIX_SOCKETS
> > static char **
> > filter_lines_with_token(char **lines, const char *token)
> > {
> > @@ -461,7 +466,6 @@ filter_lines_with_token(char **lines, const char *token)
> >
> > return result;
> > }
> > -#endif
>
> I see that you have removed "#ifndef HAVE_UNIX_SOCKETS" around the
> filter_lines_with_token() function definition so it would be always
> available, which is used to remove the @tokens@ in case user does
> not specify a default auth method for the new hostssl, hostgss
> options. I think you should also remove the "#ifndef
> HAVE_UNIX_SOCKETS" around its declaration as well so both function
> definition and declaration would make sense.
Fixed.
Thanks very much for the review!
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
