BUG #15932: Module passwordcheck doesn't reference previous hooks

On Mon, Jul 29, 2019 at 03:56:35PM +0000, PG Bug reporting form wrote:

Module passwordcheck doesn't reference (thus, doesn't call) previous
hooks:

https://github.com/postgres/postgres/blob/master/contrib/passwordcheck/passwordcheck.c#L133

As a result, if passwordcheck is used hook check_password_hook becomes
useless.

So, it would erase the trace of previous hooks if loaded last.
passwordcheck holds value as an example of use for some hooks, and
examples should be a good students, so I agree to do better and fix
that. The attached patch does the work, what do you think about it?
--
Michael

On Tue, Jul 30, 2019 at 11:24:20AM +0900, Michael Paquier wrote:

So, it would erase the trace of previous hooks if loaded last.
passwordcheck holds value as an example of use for some hooks, and
examples should be a good students, so I agree to do better and fix
that. The attached patch does the work, what do you think about it?

Are there any objections about that stuff? I would not touch anything
else than HEAD for that issue.
--
Michael

On 31 Jul 2019, at 09:15, Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Jul 30, 2019 at 11:24:20AM +0900, Michael Paquier wrote:

So, it would erase the trace of previous hooks if loaded last.
passwordcheck holds value as an example of use for some hooks, and
examples should be a good students, so I agree to do better and fix
that. The attached patch does the work, what do you think about it?

Are there any objections about that stuff? I would not touch anything
else than HEAD for that issue.

LGTM. A small nitpick is that the below comment isn’t really giving the full
picture, prev_check_password_hook is not just used for unload but for the hook
chaining.

+/* Saved hook value in case of unload */

cheers ./daniel

On Wed, Jul 31, 2019 at 09:23:54AM +0200, Daniel Gustafsson wrote:

LGTM. A small nitpick is that the below comment isn’t really giving the full
picture, prev_check_password_hook is not just used for unload but for the hook
chaining.

My understanding is that it's the same meaning, and also:
$ git grep "in case of unload"
contrib/auto_explain/auto_explain.c:/* Saved hook values in case of
unload */
contrib/pg_stat_statements/pg_stat_statements.c:/* Saved hook values
in case of unload */
src/test/modules/test_rls_hooks/test_rls_hooks.c:/* Saved hook values
in case of unload */
--
Michael

On 31 Jul 2019, at 09:34, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jul 31, 2019 at 09:23:54AM +0200, Daniel Gustafsson wrote:

LGTM. A small nitpick is that the below comment isn’t really giving the full
picture, prev_check_password_hook is not just used for unload but for the hook
chaining.

My understanding is that it's the same meaning, and also:
$ git grep "in case of unload"
contrib/auto_explain/auto_explain.c:/* Saved hook values in case of
unload */
contrib/pg_stat_statements/pg_stat_statements.c:/* Saved hook values
in case of unload */
src/test/modules/test_rls_hooks/test_rls_hooks.c:/* Saved hook values
in case of unload */

Hmm, ok, IMO my comment applies to those as well, but it may very well be me
just reading it wrong. Either way, +1 for pushing your patch.

cheers ./daniel

On Wed, Jul 31, 2019 at 09:54:17AM +0200, Daniel Gustafsson wrote:

Hmm, ok, IMO my comment applies to those as well, but it may very well be me
just reading it wrong. Either way, +1 for pushing your patch.

I am not saying that this cannot be improved either, still I am not
sure how it could be reworded. And if we do that, it should be
applied to all places where we use it. I have fixed the issue and
back-patched down to 9.4. Perhaps that was not worth caring about,
but the versions for the back-branches were not really an issue.

Thanks for the report, Rafael. And thanks for the review, Daniel.
--
Michael