Hi Zsolt,
Thanks a lot for your review comments.
>postgres.c:1076: elsewhere password_valid_until_timestamp is set to 0
>when NULL, won't that result in unintended disconnection for users?
Done. I have modified the condition check so as it will not impact users having rolvaliduntil to NULL.
>postgres.c:99: it only checks the expiration in exec_simple_query,
>shouldn't it also be part of other methods (like
>exec_execute_message)?
I missed it. In the attached patch validation is now performed across all primary query execution entry points to cover both simple and Extended query protocols.
>postgres.c:179: isn't the sys_cache_register_callback variable name a
>bit too generic, shouldn't it have a more specific name related to
>password expiration / authentication?
I agree. I have renamed the variable to a more appropriate name password_auth_cache_callback_registered.
> postgres.c:1082: the errhint text should have a period at the end.
Done.
>postgres.c:4185: The comment for CheckPasswordExpiration says that the
>function terminates the connection with FATAL, but the termination is
>actually at the call site at line 1077. Maybe it would be better to
>move that if/error inside the function, as the comment explains?
Done.
I have attached an updated patch. Request a review.
Thanks & Best Regards,
Ajit
Hello!
I noticed a few things in the patch, please consider the following:
postgres.c:1076: elsewhere password_valid_until_timestamp is set to 0
when NULL, won't that result in unintended disconnection for users?
postgres.c:99: it only checks the expiration in exec_simple_query,
shouldn't it also be part of other methods (like
exec_execute_message)?
postgres.c:179: isn't the sys_cache_register_callback variable name a
bit too generic, shouldn't it have a more specific name related to
password expiration / authentication?
postgres.c:1082: the errhint text should have a period at the end.
postgres.c:4185: The comment for CheckPasswordExpiration says that the
function terminates the connection with FATAL, but the termination is
actually at the call site at line 1077. Maybe it would be better to
move that if/error inside the function, as the comment explains?
