Joe Conway <mail@joeconway.com> writes:
> The only code specific comments were Tom's above, which have been
> addressed. If there are no serious objections I plan to commit this
> relatively soon.
I had not actually read this patchset before, but now I have, and
I have a few minor suggestions:
* The API comment for PQchangePassword should specify that encryption
is done according to the server's password_encryption setting, and
probably the SGML docs should too. You shouldn't have to read the
code to discover that.
* I don't especially care for the useless initializations of
encrypted_password, fmtpw, and fmtuser. In all those cases the
initial NULL is immediately replaced by a valid value. Probably
the compiler will figure out that the initializations are useless,
but human readers have to do so as well. Moreover, I think this
style is more bug-prone not less so, because if you ever change
the logic in a way that causes some code paths to fail to set
the variables, you won't get use-of-possibly-uninitialized-value
warnings from the compiler.
* Perhaps move the declaration of "buf" to the inner block where
it's actually used?
* This could be shortened to just "return res":
+ if (!res)
+ return NULL;
+ else
+ return res;
* I'd make the SGML documentation a bit more explicit about the
return value, say
+ Returns a <structname>PGresult</structname> pointer representing
+ the result of the <literal>ALTER USER</literal> command, or
+ a null pointer if the routine failed before issuing any command.
regards, tom lane