Re: [HACKERS] password_encryption, default and 'plain' support

On 05/05/2017 03:42 PM, Michael Paquier wrote:
> +        This option is obsolete but still accepted for backwards
> +        compatibility.
> Isn't that incorrect English? It seems to me that this be non-plural,
> as "for backward compatibility".

I changed most cases to "backward compatibility", except the one in 
create_role.sgml, because there were other instances of "backwards 
compatibility" on that page, and I didn't want this to stick out.

> The comment at the top of check_password() in passwordcheck.c does not
> mention scram, you may want to update that.

Reworded the comment, to not list all the possible values.

> +                       /*
> +                        * We never store passwords in plaintext, so
> this shouldn't
> +                        * happen.
> +                        */
>                         break;
> An error here is overthinking?

It's not shown in the diff's context, but an error is returned just 
after the switch statement. I considered leaving out the "case 
PASSWORD_TYPE_PLAINTEXT" altogether, but then you might get compiler 
warnings complaining that that enum value is not handled. I also 
considered putting a an explicit "default:" there, which returns an 
error, but then you'd again miss out on compiler warnings, if you add a 
new password hash type and forget to add a case here to handle it.

>  -- consistency of password entries
> -SET password_encryption = 'plain';
> -CREATE ROLE regress_passwd1 PASSWORD 'role_pwd1';
>  SET password_encryption = 'md5';
> Nit: this is skipping directly to role number 2.

Renumbered the test roles.

Committed with those little cleanups. Thanks for the review!

- Heikki