MIME-Version: 1.0
References: 
 <CALj2ACXPorUqePswDtOeM_s82v9RW32E1fYmOPZ5NuE+TWKj_A@mail.gmail.com>
 <13156732-5189-af54-3d28-e2ee2a8568e0@oss.nttdata.com>
 <CALj2ACV9NrMDT5Ndpm5STUYRBfKQJoPOgA5a27a1XRc4h8wVYw@mail.gmail.com>
 <069fb3bc-3907-4d85-19fd-e4b2d2f004b4@oss.nttdata.com>
 <CALj2ACXoCsVFFMi4RAPE-OpWOegS8u5+y_DCP3TXi7GeG61gyg@mail.gmail.com>
 <CALj2ACX0JkSM2BhiLog1xP9x2tsdjVdL8U-=OABf2YzYS+ywpQ@mail.gmail.com>
 <CALj2ACVCnHLdSRSZJ9nkPABcLGWm=Bvj11ajuAhgU6canRJsqA@mail.gmail.com>
 <CAGRY4nxsAe_1k_9g5b47orA0S011iBoHsXHFMH7cg7HV0O1bwQ@mail.gmail.com>
In-Reply-To: 
 <CAGRY4nxsAe_1k_9g5b47orA0S011iBoHsXHFMH7cg7HV0O1bwQ@mail.gmail.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 17 Nov 2020 17:48:38 +0530
Message-ID: 
 <CALj2ACWWy1YcngpCUn09AsXMfOzwjfNqbVosfoRY0vhhVWhVBw@mail.gmail.com>
Subject: Re: Use standard SIGHUP and SIGTERM handlers in autoprewarm module
To: Craig Ringer <craig.ringer@enterprisedb.com>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Robert Haas <robert.haas@enterprisedb.com>
Content-Type: multipart/mixed; boundary="00000000000057e3b105b44c7fd8"
Precedence: bulk

--00000000000057e3b105b44c7fd8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Thanks Craig!

On Fri, Oct 23, 2020 at 9:37 AM Craig Ringer
<craig.ringer@enterprisedb.com> wrote:
>
> src/test/modules/test_shm_mq/worker.c appears to do the right thing the w=
rong way - it has its own custom handler instead of using die() or SignalHa=
ndlerForShutdownRequest().
>

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 !=3D 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.

>
> In contrast  src/test/modules/worker_spi/worker_spi.c looks plain wrong. =
Especially since it's quoted as an example of how to do things right. It wo=
n't respond to SIGTERM at all while it's executing a query from its queue, =
no matter how long that query takes or whether it blocks. It can inhibit ev=
en postmaster shutdown as a result.
>

Postmaster sends SIGTERM to all children(backends and bgworkers) for
both smart shutdown(wait for children to end their work, then shut
down.) and fast shutdown(rollback active transactions and shutdown
when they are gone.) For immediate shutdown SIGQUIT is sent to
children.(see pmdie()).

For each bg worker(so is for worker_spi.c),
SignalHandlerForCrashExit() is set as SIGQUIT handler in
InitPostmasterChild(), which does nothing but exits immediately. We
can not use quickdie() here because as a bg worker we don't have
to/can not send anything to client. We are good with
SignalHandlerForCrashExit() as with all other bg workers.

Yes, if having worker_spi_sigterm/SignalHandlerForShutdownRequest,
sometimes(as explained above) the worker_spi worker may not respond to
SIGTERM. I think we should be having die() as SIGTERM handler here (as
with normal backend and parallel workers).

Attaching another patch that has replaced custom SIGTERM handler
worker_spi_sigterm() with die() and custom SIGHUP handler
worker_spi_sighup() with standard SignalHandlerForConfigReload().

>
> I was going to lob off a quick patch to fix this by making both use quick=
die() for SIGQUIT and die() for SIGTERM, but after reading for a bit I'm no=
 longer sure what the right choice even is. I'd welcome some opinions.
>

We can not use quickdie() here because as a bg worker we don't have
to/can not send anything to client. We are good with
SignalHandlerForCrashExit() as with all other bg workers.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

--00000000000057e3b105b44c7fd8
Content-Type: application/octet-stream;
	name="v1-0001-Use-die-instead-of-custom-signal-handler-in-test_shm_mq-worker.patch"
Content-Disposition: attachment;
	filename="v1-0001-Use-die-instead-of-custom-signal-handler-in-test_shm_mq-worker.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_khlxl3ko0>
X-Attachment-Id: f_khlxl3ko0

RnJvbSBlODliNjg1MDJkNGZhN2M4NGNlNjM3YzI3YTgxZWI1MDJiNTFlYWZmIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBCaGFyYXRoIFJ1cGlyZWRkeSA8YmhhcmF0aC5ydXBpcmVkZHlA
ZW50ZXJwcmlzZWRiLmNvbT4KRGF0ZTogVHVlLCAxNyBOb3YgMjAyMCAxNzozMzoyNSArMDUzMApT
dWJqZWN0OiBbUEFUQ0ggdjFdIFVzZSBkaWUoKSBpbnN0ZWFkIG9mIGN1c3RvbSBzaWduYWwgaGFu
ZGxlciBpbiB0ZXN0X3NobV9tcQogd29ya2VyCgpEb2luZ0NvbW1hbmRSZWFkIGNhbiBuZXZlciBi
ZSB0cnVlIGluIGJnIHdvcmtlcnMgYXMgaXQncyBiZWluZyBzZXQKdG8gdHJ1ZSBvbmx5IGluIG5v
cm1hbCBiYWNrZW5kIFBvc3RncmVzTWFpbigpLiBJZiB3ZSB1c2UgZGllKCkKKGluc3RlYWQgb2Yg
Y3VzdG9tIFNJR1RFUk0gaGFuZGxlcnMgc3VjaCBhcyBoYW5kbGVfc2lndGVybSgpKSBpbiBhCmJn
IHdvcmtlci9ub24tYmFja2VuZCBwcm9jZXNzLCB0aGVyZSBhcmUgbm8gY2hhbmNlcyB0aGF0IHRo
ZQpEb2luQ29tbWFuZFJFYWQgcGFydCBvZiB0aGUgY29kZSBnZXRzIGhpdCBhbmQgdGhlIGludGVy
cnVwdHMgYXJlCnByb2Nlc3NlZCBpbW1lZGlhdGVseS4gQW5kIGFsc28gaGVyZSBhcmUgdGhlIGJn
IHdvcmtlciBwcm9jZXNzCnRoYXQgdXNlIGRpZSgpIGluc3RlYWQgb2YgdGhlaXIgb3duIGN1c3Rv
bSBoYW5kbGVyczoKcGFyYWxsZWwgd29ya2VycywgYXV0b3ByZXdhcm0gd29ya2VyLCBhdXRvdmFj
dXVtIHdvcmtlciwKbG9naWNhbCByZXBsaWNhdGlvbiBsYXVuY2hlciBhbmQgYXBwbHkgd29ya2Vy
LCB3YWwgc2VuZGVyIHByb2Nlc3MKLS0tCiBzcmMvdGVzdC9tb2R1bGVzL3Rlc3Rfc2htX21xL3dv
cmtlci5jIHwgMjggKysrLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCiAxIGZpbGUgY2hhbmdlZCwg
MyBpbnNlcnRpb25zKCspLCAyNSBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvdGVzdC9t
b2R1bGVzL3Rlc3Rfc2htX21xL3dvcmtlci5jIGIvc3JjL3Rlc3QvbW9kdWxlcy90ZXN0X3NobV9t
cS93b3JrZXIuYwppbmRleCBjYjYzYWRlYmVlLi4zZjJlOWJmODEyIDEwMDY0NAotLS0gYS9zcmMv
dGVzdC9tb2R1bGVzL3Rlc3Rfc2htX21xL3dvcmtlci5jCisrKyBiL3NyYy90ZXN0L21vZHVsZXMv
dGVzdF9zaG1fbXEvd29ya2VyLmMKQEAgLTI0LDEwICsyNCwxMCBAQAogI2luY2x1ZGUgInN0b3Jh
Z2UvcHJvY2FycmF5LmgiCiAjaW5jbHVkZSAic3RvcmFnZS9zaG1fbXEuaCIKICNpbmNsdWRlICJz
dG9yYWdlL3NobV90b2MuaCIKKyNpbmNsdWRlICJ0Y29wL3Rjb3Bwcm90LmgiCiAKICNpbmNsdWRl
ICJ0ZXN0X3NobV9tcS5oIgogCi1zdGF0aWMgdm9pZCBoYW5kbGVfc2lndGVybShTSUdOQUxfQVJH
Uyk7CiBzdGF0aWMgdm9pZCBhdHRhY2hfdG9fcXVldWVzKGRzbV9zZWdtZW50ICpzZWcsIHNobV90
b2MgKnRvYywKIAkJCQkJCQkgaW50IG15d29ya2VybnVtYmVyLCBzaG1fbXFfaGFuZGxlICoqaW5x
aHAsCiAJCQkJCQkJIHNobV9tcV9oYW5kbGUgKipvdXRxaHApOwpAQCAtNTgsMTAgKzU4LDkgQEAg
dGVzdF9zaG1fbXFfbWFpbihEYXR1bSBtYWluX2FyZykKIAkgKiBFc3RhYmxpc2ggc2lnbmFsIGhh
bmRsZXJzLgogCSAqCiAJICogV2Ugd2FudCBDSEVDS19GT1JfSU5URVJSVVBUUygpIHRvIGtpbGwg
b2ZmIHRoaXMgd29ya2VyIHByb2Nlc3MganVzdCBhcwotCSAqIGl0IHdvdWxkIGEgbm9ybWFsIHVz
ZXIgYmFja2VuZC4gIFRvIG1ha2UgdGhhdCBoYXBwZW4sIHdlIGVzdGFibGlzaCBhCi0JICogc2ln
bmFsIGhhbmRsZXIgdGhhdCBpcyBhIHN0cmlwcGVkLWRvd24gdmVyc2lvbiBvZiBkaWUoKS4KKwkg
KiBpdCB3b3VsZCBhIG5vcm1hbCB1c2VyIGJhY2tlbmQuICBUbyBtYWtlIHRoYXQgaGFwcGVuLCB3
ZSB1c2UgZGllKCkuCiAJICovCi0JcHFzaWduYWwoU0lHVEVSTSwgaGFuZGxlX3NpZ3Rlcm0pOwor
CXBxc2lnbmFsKFNJR1RFUk0sIGRpZSk7CiAJQmFja2dyb3VuZFdvcmtlclVuYmxvY2tTaWduYWxz
KCk7CiAKIAkvKgpAQCAtMTk2LDI0ICsxOTUsMyBAQCBjb3B5X21lc3NhZ2VzKHNobV9tcV9oYW5k
bGUgKmlucWgsIHNobV9tcV9oYW5kbGUgKm91dHFoKQogCQkJYnJlYWs7CiAJfQogfQotCi0vKgot
ICogV2hlbiB3ZSByZWNlaXZlIGEgU0lHVEVSTSwgd2Ugc2V0IEludGVycnVwdFBlbmRpbmcgYW5k
IFByb2NEaWVQZW5kaW5nIGp1c3QKLSAqIGxpa2UgYSBub3JtYWwgYmFja2VuZC4gIFRoZSBuZXh0
IENIRUNLX0ZPUl9JTlRFUlJVUFRTKCkgd2lsbCBkbyB0aGUgcmlnaHQKLSAqIHRoaW5nLgotICov
Ci1zdGF0aWMgdm9pZAotaGFuZGxlX3NpZ3Rlcm0oU0lHTkFMX0FSR1MpCi17Ci0JaW50CQkJc2F2
ZV9lcnJubyA9IGVycm5vOwotCi0JU2V0TGF0Y2goTXlMYXRjaCk7Ci0KLQlpZiAoIXByb2NfZXhp
dF9pbnByb2dyZXNzKQotCXsKLQkJSW50ZXJydXB0UGVuZGluZyA9IHRydWU7Ci0JCVByb2NEaWVQ
ZW5kaW5nID0gdHJ1ZTsKLQl9Ci0KLQllcnJubyA9IHNhdmVfZXJybm87Ci19Ci0tIAoyLjI1LjEK
Cg==
--00000000000057e3b105b44c7fd8
Content-Type: application/octet-stream;
	name="v1-0001-Use-standard-SIGHUP-handler-and-SIGTERM-handler-die-in-worker_spi.patch"
Content-Disposition: attachment;
	filename="v1-0001-Use-standard-SIGHUP-handler-and-SIGTERM-handler-die-in-worker_spi.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_khlxl8nz1>
X-Attachment-Id: f_khlxl8nz1

RnJvbSA2NzMxNjc3ZjUyOTUwOWFiYjg3ODI1MTA0YTViN2JjMWRmOTM5NjUwIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBCaGFyYXRoIFJ1cGlyZWRkeSA8YmhhcmF0aC5ydXBpcmVkZHlA
ZW50ZXJwcmlzZWRiLmNvbT4KRGF0ZTogVHVlLCAxNyBOb3YgMjAyMCAxNzoyMjo0NyArMDUzMApT
dWJqZWN0OiBbUEFUQ0ggdjFdIFVzZSBzdGFuZGFyZCBTSUdIVVAgaGFuZGxlciBhbmQgU0lHVEVS
TSBoYW5kbGVyIGRpZSBpbgogd29ya2VyX3NwaQoKLS0tCiBzcmMvdGVzdC9tb2R1bGVzL3dvcmtl
cl9zcGkvd29ya2VyX3NwaS5jIHwgNTIgKysrKy0tLS0tLS0tLS0tLS0tLS0tLS0tCiAxIGZpbGUg
Y2hhbmdlZCwgOSBpbnNlcnRpb25zKCspLCA0MyBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9z
cmMvdGVzdC9tb2R1bGVzL3dvcmtlcl9zcGkvd29ya2VyX3NwaS5jIGIvc3JjL3Rlc3QvbW9kdWxl
cy93b3JrZXJfc3BpL3dvcmtlcl9zcGkuYwppbmRleCAyNTgyMzdmOWJmLi43MWY5NTUyMDZhIDEw
MDY0NAotLS0gYS9zcmMvdGVzdC9tb2R1bGVzL3dvcmtlcl9zcGkvd29ya2VyX3NwaS5jCisrKyBi
L3NyYy90ZXN0L21vZHVsZXMvd29ya2VyX3NwaS93b3JrZXJfc3BpLmMKQEAgLTI1LDYgKzI1LDcg
QEAKIC8qIFRoZXNlIGFyZSBhbHdheXMgbmVjZXNzYXJ5IGZvciBhIGJnd29ya2VyICovCiAjaW5j
bHVkZSAibWlzY2FkbWluLmgiCiAjaW5jbHVkZSAicG9zdG1hc3Rlci9iZ3dvcmtlci5oIgorI2lu
Y2x1ZGUgInBvc3RtYXN0ZXIvaW50ZXJydXB0LmgiCiAjaW5jbHVkZSAic3RvcmFnZS9pcGMuaCIK
ICNpbmNsdWRlICJzdG9yYWdlL2xhdGNoLmgiCiAjaW5jbHVkZSAic3RvcmFnZS9sd2xvY2suaCIK
QEAgLTQ4LDEwICs0OSw2IEBAIFBHX0ZVTkNUSU9OX0lORk9fVjEod29ya2VyX3NwaV9sYXVuY2gp
Owogdm9pZAkJX1BHX2luaXQodm9pZCk7CiB2b2lkCQl3b3JrZXJfc3BpX21haW4oRGF0dW0pIHBn
X2F0dHJpYnV0ZV9ub3JldHVybigpOwogCi0vKiBmbGFncyBzZXQgYnkgc2lnbmFsIGhhbmRsZXJz
ICovCi1zdGF0aWMgdm9sYXRpbGUgc2lnX2F0b21pY190IGdvdF9zaWdodXAgPSBmYWxzZTsKLXN0
YXRpYyB2b2xhdGlsZSBzaWdfYXRvbWljX3QgZ290X3NpZ3Rlcm0gPSBmYWxzZTsKLQogLyogR1VD
IHZhcmlhYmxlcyAqLwogc3RhdGljIGludAl3b3JrZXJfc3BpX25hcHRpbWUgPSAxMDsKIHN0YXRp
YyBpbnQJd29ya2VyX3NwaV90b3RhbF93b3JrZXJzID0gMjsKQEAgLTY0LDM4ICs2MSw2IEBAIHR5
cGVkZWYgc3RydWN0IHdvcmt0YWJsZQogCWNvbnN0IGNoYXIgKm5hbWU7CiB9IHdvcmt0YWJsZTsK
IAotLyoKLSAqIFNpZ25hbCBoYW5kbGVyIGZvciBTSUdURVJNCi0gKgkJU2V0IGEgZmxhZyB0byBs
ZXQgdGhlIG1haW4gbG9vcCB0byB0ZXJtaW5hdGUsIGFuZCBzZXQgb3VyIGxhdGNoIHRvIHdha2UK
LSAqCQlpdCB1cC4KLSAqLwotc3RhdGljIHZvaWQKLXdvcmtlcl9zcGlfc2lndGVybShTSUdOQUxf
QVJHUykKLXsKLQlpbnQJCQlzYXZlX2Vycm5vID0gZXJybm87Ci0KLQlnb3Rfc2lndGVybSA9IHRy
dWU7Ci0JU2V0TGF0Y2goTXlMYXRjaCk7Ci0KLQllcnJubyA9IHNhdmVfZXJybm87Ci19Ci0KLS8q
Ci0gKiBTaWduYWwgaGFuZGxlciBmb3IgU0lHSFVQCi0gKgkJU2V0IGEgZmxhZyB0byB0ZWxsIHRo
ZSBtYWluIGxvb3AgdG8gcmVyZWFkIHRoZSBjb25maWcgZmlsZSwgYW5kIHNldAotICoJCW91ciBs
YXRjaCB0byB3YWtlIGl0IHVwLgotICovCi1zdGF0aWMgdm9pZAotd29ya2VyX3NwaV9zaWdodXAo
U0lHTkFMX0FSR1MpCi17Ci0JaW50CQkJc2F2ZV9lcnJubyA9IGVycm5vOwotCi0JZ290X3NpZ2h1
cCA9IHRydWU7Ci0JU2V0TGF0Y2goTXlMYXRjaCk7Ci0KLQllcnJubyA9IHNhdmVfZXJybm87Ci19
Ci0KIC8qCiAgKiBJbml0aWFsaXplIHdvcmtzcGFjZSBmb3IgYSB3b3JrZXIgcHJvY2VzczogY3Jl
YXRlIHRoZSBzY2hlbWEgaWYgaXQgZG9lc24ndAogICogYWxyZWFkeSBleGlzdC4KQEAgLTE3OSw4
ICsxNDQsOCBAQCB3b3JrZXJfc3BpX21haW4oRGF0dW0gbWFpbl9hcmcpCiAJdGFibGUtPm5hbWUg
PSBwc3RyZHVwKCJjb3VudGVkIik7CiAKIAkvKiBFc3RhYmxpc2ggc2lnbmFsIGhhbmRsZXJzIGJl
Zm9yZSB1bmJsb2NraW5nIHNpZ25hbHMuICovCi0JcHFzaWduYWwoU0lHSFVQLCB3b3JrZXJfc3Bp
X3NpZ2h1cCk7Ci0JcHFzaWduYWwoU0lHVEVSTSwgd29ya2VyX3NwaV9zaWd0ZXJtKTsKKwlwcXNp
Z25hbChTSUdIVVAsIFNpZ25hbEhhbmRsZXJGb3JDb25maWdSZWxvYWQpOworCXBxc2lnbmFsKFNJ
R1RFUk0sIGRpZSk7CiAKIAkvKiBXZSdyZSBub3cgcmVhZHkgdG8gcmVjZWl2ZSBzaWduYWxzICov
CiAJQmFja2dyb3VuZFdvcmtlclVuYmxvY2tTaWduYWxzKCk7CkBAIC0yMTksOSArMTg0LDEwIEBA
IHdvcmtlcl9zcGlfbWFpbihEYXR1bSBtYWluX2FyZykKIAkJCQkJIHRhYmxlLT5uYW1lKTsKIAog
CS8qCi0JICogTWFpbiBsb29wOiBkbyB0aGlzIHVudGlsIHRoZSBTSUdURVJNIGhhbmRsZXIgdGVs
bHMgdXMgdG8gdGVybWluYXRlCisJICogTWFpbiBsb29wOiBkbyB0aGlzIHVudGlsIFNJR1RFUk0g
aXMgcmVjZWl2ZWQgYW5kIHByb2Nlc3NlZCBieQorCSAqIFByb2Nlc3NJbnRlcnJ1cHRzLgogCSAq
LwotCXdoaWxlICghZ290X3NpZ3Rlcm0pCisJZm9yICg7OykKIAl7CiAJCWludAkJCXJldDsKIApA
QCAtMjQyLDkgKzIwOCw5IEBAIHdvcmtlcl9zcGlfbWFpbihEYXR1bSBtYWluX2FyZykKIAkJLyoK
IAkJICogSW4gY2FzZSBvZiBhIFNJR0hVUCwganVzdCByZWxvYWQgdGhlIGNvbmZpZ3VyYXRpb24u
CiAJCSAqLwotCQlpZiAoZ290X3NpZ2h1cCkKKwkJaWYgKENvbmZpZ1JlbG9hZFBlbmRpbmcpCiAJ
CXsKLQkJCWdvdF9zaWdodXAgPSBmYWxzZTsKKwkJCUNvbmZpZ1JlbG9hZFBlbmRpbmcgPSBmYWxz
ZTsKIAkJCVByb2Nlc3NDb25maWdGaWxlKFBHQ19TSUdIVVApOwogCQl9CiAKQEAgLTMwMyw3ICsy
NjksNyBAQCB3b3JrZXJfc3BpX21haW4oRGF0dW0gbWFpbl9hcmcpCiAJCXBnc3RhdF9yZXBvcnRf
YWN0aXZpdHkoU1RBVEVfSURMRSwgTlVMTCk7CiAJfQogCi0JcHJvY19leGl0KDEpOworCS8qIE5v
dCByZWFjaGFibGUgKi8KIH0KIAogLyoKLS0gCjIuMjUuMQoK
--00000000000057e3b105b44c7fd8--