Re: Hooks for session start and end, take two

Hi,

Moving the discussion of the issues leading to this being reverted back
to -hackers:

On 2019-10-02 23:52:31 +0900, Fujii Masao wrote:
> On Wed, Oct 2, 2019 at 10:08 PM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> >
> > On 2019-Oct-02, Michael Paquier wrote:
> >
> > > On Wed, Oct 02, 2019 at 01:27:50PM +0900, Fujii Masao wrote:
> > > > If only session end hook is problematic, you will commit session start
> > > > hook again?
> > >
> > > Sure, it would be possible to cut the apple in half here. Now my
> > > understanding was that both hooks were a set. What do people think?
> >
> > I think that having just session start is still useful

> +1

Well, I think the patch actually needs to do a lot more design work to
be comittable and usable. It's very unclear how these hooks are actually
supposed to be used safely. Issues I see:

- the startup hook isn't actually guaranteed to be able to write
anything, we might be in hot-standby
- the startup hook doesn't just run in normal sessions, it also runs in
walsenders, including ones not connected to a database. But for some
implementation reasons it won't run for background workers.
- the shutdown hook placement means that any error triggered within the
handler probably cannot be cleaned up properly anymore
- do we actually want to run code like this for e.g. FATAL errors?
- THERE IS NOT A SINGLE COMMENT EXPLAINING WHAT CAN BE SAFELY DONE IN THESE
HOOKS. In fact, outside of the tests, the only comments in this are:
/* Hook for plugins to get control at start and end of session */
/* Hook for plugins to get control at start of session */
/* Hook at session end */
/* Hook for plugins to get control at end of session */

> Regarding session end hook, you can do the almost same thing as that hook
> by calling on_shmem_exit(), before_shmem_exit() or on_proc_exit() in
> other hook like session start hook. This approach also has the same issue
> discussed upthread, though. Anyway, I'm not sure if session end hook is
> "actually" necessary.

No, it's not actually the same (at least for
before_shmem_exit). ShutdownPostgres() runs deliberately as the *last*
before_shmem_exit to run before we switch over to tearing down shmem:

/*
* Set up process-exit callback to do pre-shutdown cleanup. This is the
* first before_shmem_exit callback we register; thus, this will be the
* last thing we do before low-level modules like the buffer manager begin
* to close down. We need to have this in place before we begin our first
* transaction --- if we fail during the initialization transaction, as is
* entirely possible, we need the AbortTransaction call to clean up.
*/

*
* User-level cleanup, such as temp-relation removal and UNLISTEN, happens
* via separate callbacks that execute before this one. We don't combine the
* callbacks because we still want this one to happen if the user-level
* cleanup fails.

- Andres