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@2ndquadrant.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