Re: [PATCH] A hook for session start

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,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello