Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module

On 2020/11/20 19:33, Bharath Rupireddy wrote:
> On Wed, Nov 18, 2020 at 5:52 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com <mailto:masao(dot)fujii(at)oss(dot)nttdata(dot)com>> wrote:
> >
> > > handle_sigterm() and die() are similar except that die() has extra
> > > handling(below) for exiting immediately when waiting for input/command
> > > from the client.
> > >      /*
> > >       * If we're in single user mode, we want to quit immediately - we can't
> > >       * rely on latches as they wouldn't work when stdin/stdout is a file.
> > >       * Rather ugly, but it's unlikely to be worthwhile to invest much more
> > >       * effort just for the benefit of single user mode.
> > >       */
> > >      if (DoingCommandRead && whereToSendOutput != DestRemote)
> > >          ProcessInterrupts();
> > >
> > > Having this extra handling is correct for normal backends as they can
> > > connect directly to clients for reading input commands, the above if
> > > condition may become true and ProcessInterrupts() may be called to
> > > handle the SIGTERM immediately.
> > >
> > > Note that DoingCommandRead can never be true in bg workers as it's
> > > being set to true only in normal backend PostgresMain(). If we use
> > > die()(instead of custom SIGTERM handlers such as handle_sigterm()) in
> > > a bg worker/non-backend process, there are no chances that the above
> > > part of the code gets hit and the interrupts are processed
> > > immediately. And also here are the bg worker process that use die()
> > > instead of their own custom handlers: parallel workers, autoprewarm
> > > worker, autovacuum worker, logical replication launcher and apply
> > > worker, wal sender process
> > >
> > > I think we can also use die() instead of handle_sigterm() in
> > > test_shm_mq/worker.c.
> > >
> > > I attached a patch for this change.
> >
> > Thanks for the patch! It looks good to me.
> >
>
> Thanks.

When I read the patch again, I found that, with the patch, the shutdown
of worker_spi causes to report the following FATAL message.

FATAL: terminating connection due to administrator command

Isn't this message confusing because it's not a connection? If so,
we need to update ProcessInterrupts() so that the proper message is
reported like other bgworkers do.

BTW, though this is not directly related to this topic, it's strange to me
that the normal shutdown of bgworker causes to emit FATAL message
and the message "background worker ... exited with exit code 1"
at the normal shutdown. I've heard sometimes that users coplained that
it's confusing to report that message for logical replication launcher
at the normal shutdown. Maybe the mechanism to shutdown bgworker
normally seems to have not been implemented well yet.

Without the patch, since worker_spi cannot handle the shutdown request
promptly while it's executing the backend code, maybe we can say this is
a bug. But worker_spi is a just example of the code, I'm not thinking to
do back-patch for now. Thought?

>
> >
> > BTW, I tried to find the past discussion about why die() was not used,
> > but I could not yet.
> >
>
> I think the commit 2505ce0 that altered the comment has something:

Thanks for the info! Will check that.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION