From: | Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Aleksandr Parfenov <a(dot)parfenov(at)postgrespro(dot)ru>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> |
Subject: | Re: [PATCH] A hook for session start |
Date: | 2017-11-08 17:42:43 |
Message-ID: | CAFcNs+q7x8BTPqS-rRX_gWByfsMmS8JMi-CjUquafXyJiPHY=A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
session_hooks_v7.patch | text/x-patch | 10.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Oleg Bartunov | 2017-11-08 18:46:49 | Re: need info about extensibility in other databases |
Previous Message | Merlin Moncure | 2017-11-08 17:15:02 | Re: SQL procedures |