On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <
michael.paquier@gmail.com> wrote:
>
> + /* Hook just normal backends */
> + if (session_end_hook && MyBackendId != InvalidBackendId)
> + (*session_end_hook) ();
> I have been wondering about the necessity of this restriction.
> Couldn't it be useful to just let the people implementing the hook do
> any decision-making? Tracking some non-backend shutdowns may be useful
> as well for logging purposes.
>
Also makes sense... I move down this check to hook code.
> The patch is beginning to take shape (I really like the test module
> you are implementing here!), still needs a bit more work.
>
Thanks... and your review is helping a lot!!!
> +CREATE ROLE session_hook_usr1 LOGIN;
> +CREATE ROLE session_hook_usr2 LOGIN;
> Roles created during regression tests should be prefixed with regress_.
>
Fixed.
> + if (prev_session_start_hook)
> + prev_session_start_hook();
> +
> + if (session_start_hook_enabled)
> + (void) register_session_hook("START");
> Shouldn't both be reversed?
>
Fixed.
> +/* sample sessoin end hook function */
> Typo here.
>
Fixed.
> +CREATE DATABASE session_hook_db;
> [...]
> + if (!strcmp(dbname, "session_hook_db"))
> + {
> Creating a database is usually avoided in sql files as those can be
> run on existing servers using installcheck. I am getting that this
> restriction is in place so as it is possible to create an initial
> connection to the server so as the base table to log the activity is
> created. That's a fine assumption to me. Still, I think that the
> following changes should be done:
> - Let's restrict the logging to a role name instead of a database
> name, and let's parametrize it with a setting in the temporary
> configuration file. Let's not bother about multiple role support with
> a list, for the sake of tests and simplicity only defining one role
> looks fine to me. Comments in the code should be clear about the
> dependency.
Makes sense and simplify the test code. Fixed.
> - The GUCs test_session_hooks.start_enabled and
> test_session_hooks.end_enabled are actually redundant with the
> database restriction (well, role restriction per previous comment), so
> let's remove them. Please let's also avoid ALTER SYSTEM calls in tests
> as it would impact existing installations with installcheck.
>
Also simplify the test code. Fixed.
> Impossible to tell committer's feeling about this test suite, but my
> opinion is to keep it as that's a good template example about what can
> be done with those two hooks.
>
+1
Attached new version.
Regards,
