Re: Hooks for session start and end, take two

Hi,

On 2019-10-03 11:03:39 +0900, Michael Paquier wrote:
> On Wed, Oct 02, 2019 at 10:23:54AM -0700, Andres Freund wrote:
> > - the startup hook isn't actually guaranteed to be able to write
> > anything, we might be in hot-standby
>
> Right.
>
> > - 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 idea here is to have a hook which can be triggered at the start of
> a process which can be externally triggered, which I guess is normal
> even for WAL senders not connected to a database.

But what can you do in that situation? Without a database connection,
for example, you better not consider inserting anything into a table.

> > - do we actually want to run code like this for e.g. FATAL errors?
>
> That was the intention.

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.

> > - 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 */
>
> We include in the backend code a dozen of hooks or so similar to this
> one (excluding from the count GUC and reloption hooks and such). For
> most of them we do not provide much extensive documentation nor
> explanations similar to that

I don't mean sgml docs, but rather code comments. Most hooks don't have
a caveat list as long as the hooks proposed here. And for most the
specific location they hook into is more obvious: Whereas e.g. planner
hook can just happen when planning happens, the proposed hooks here
could plausible be put into many different places.

If somebody had bothered to add an explanation as to why the hook
placements are a good idea, they might have noticed that the location
the shutdown hook was placed at was terrible.

> so it is rather unclear what would be a minimum and acceptable set of
> comments and/or documentation which would be fine (the same comment
> could basically apply to the planner or post-parse hooks).

Meh. As described above, I don't think most other hooks are as vague and
dangerous as these. Nor is the existance of underdocumented code an
argument for adding more.

> Should we finally add a specific section in the user-visible docs even
> if there has been reluctance to do so? My guess is that we go down to
> this kind of requirement if we want to be able to never forget to add
> documentation for any kind of new hook.

This seems like an entirely separate discussion.

Greetings,

Andres Freund