Re: Hooks for session start and end, take two

Hi,

On 2019-10-03 13:20:09 +0900, Michael Paquier wrote:
> On Wed, Oct 02, 2019 at 09:09:53PM -0700, Andres Freund wrote:
> > I have *SERIOUS* problems with performing additional writing
> > transactions in case of a FATAL. Something might have thrown that for a
> > reason, and just carrying on executing an arbitrary amount of code, this
> > might even involve triggers etc, imo seriously is not OK.
>
> Well, hook authors can do a lot of stupid things..

I don't have much problems if they knowingly do stupid stuff. But if
what we provide basically only consists out of pitfalls, than we need to
reconsider the interface, or at least have guidance on how to avoid
them.

> Anyway it looks that the end hook is out of scope as far as the
> discussion has gone based on the lack of facility, and that there is
> still interest for the start hook.

I don't really think that's a fair summary of what's been discussed.

> diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
> index e8d8e6f828..6d80cc2d64 100644
> --- a/src/backend/tcop/postgres.c
> +++ b/src/backend/tcop/postgres.c
> @@ -171,6 +171,9 @@ static ProcSignalReason RecoveryConflictReason;
> static MemoryContext row_description_context = NULL;
> static StringInfoData row_description_buf;
>
> +/* Hook for plugins to get control at start of session */
> +session_start_hook_type session_start_hook = NULL;
> +
> /* ----------------------------------------------------------------
> * decls for routines only used in this file
> * ----------------------------------------------------------------
> @@ -3968,6 +3971,9 @@ PostgresMain(int argc, char *argv[],
> if (!IsUnderPostmaster)
> PgStartTime = GetCurrentTimestamp();
>
> + if (session_start_hook)
> + (*session_start_hook) ();
> +
> /*
> * POSTGRES main processing loop begins here
> *
> diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
> index ec21f7e45c..f42935f019 100644
> --- a/src/include/tcop/tcopprot.h
> +++ b/src/include/tcop/tcopprot.h
> @@ -30,6 +30,10 @@ extern PGDLLIMPORT const char *debug_query_string;
> extern int max_stack_depth;
> extern int PostAuthDelay;
>
> +/* Hook for plugins to get control at start session */
> +typedef void (*session_start_hook_type) (void);
> +extern PGDLLIMPORT session_start_hook_type session_start_hook;
> +
> /* GUC-configurable parameters */

In this state I think this patch should be flat out rejected.

I'm seriously baffled at how this stuff is being pursued aggressively,
quite apparently without any serious design considerations. I mean this
stuff had to be backed out twice, yet it's being reproposed without much
consideration for concerns?

- Andres