Re: background worker shudown (was Re: [HACKERS] Why does logical replication launcher exit with exit code 1?)

On Wed, Oct 10, 2018 at 12:00 AM Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> So I suppose we should just remove it, with something like 0002. I'm
> a bit uneasy about existing code out there that might be not calling
> CFI. OTOH I suspect that a lot of code copied worker_spi.c and
> installed its own handler.

I agree -- I think worker_spi.c has probably been copied a lot, and
that's not a good thing.

> Maybe src/test/modules/worker_spi.c shouldn't use approach #1 (even if
> it might technically be OK for that code)? I think I might have been
> guilty of copying that.

+1. It's not *really* OK unless all of the SQL queries it executes
are super-short and can't possibly block ... which I guess is never
really true of any SQL queries, right?

> > Maybe we could replace this by a general-purpose hook. So instead of
> > the various tests for process types that are there now, we would just
> > have
> >
> > if (procdie_hook != NULL)
> > (*procdie_hook)();
> >
> > And that hook can do whatever you like (but probably including dying).
>
> Ok, patch 0001 is a sketch like that, for discussion.

I was assuming that we'd replace the existing message-selection logic
with customer proc-die handlers. Also, I think we should not indicate
in the comments that the handler is expected to proc_exit() or
ereport(FATAL), because you might want to do something else there,
then return and let the usual thing happen.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company