Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 29 Dec 2025 11:03:39 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Ryo Matsumura (Fujitsu)" <matsumura.ryo@fujitsu.com>
Cc: "Aya Iwata (Fujitsu)" <iwata.aya@fujitsu.com>,
	'Pavel Stehule' <pavel.stehule@gmail.com>,
	Peter Smith <smithpb2250@gmail.com>,
	Chao Li <li.evan.chao@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: [PROPOSAL] Termination of Background Workers for ALTER/DROP
 DATABASE
Message-ID: <aVHhew6_9O7rD4RR@paquier.xyz>
References: <aPBsd2e2SqLs0Kdk@paquier.xyz>
 <CAFj8pRB_FH-Pcth9XFcpY2OTasVOP-2DfYOsVxL38igw0O4hdg@mail.gmail.com>
 <OS7PR01MB119647141272DBF9364A1C5AAEAADA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <CAFj8pRCRnN4SyZDPbQwTyKZ_kVHfwLG6udCXsXDTG_z9fWwYQg@mail.gmail.com>
 <OS7PR01MB119648203BD748ED008FA5BF5EAABA@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <CAFj8pRCn0=jn4yaeg1JoxxxnUNeXm1KCouES8Puq_GgBsXrNTQ@mail.gmail.com>
 <OS7PR01MB1196405302B700736580DEE49EAA8A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <CAFj8pRBk7zTKoxAAKAihNQuimztniWHPyRWHKp2iHq_2hH+dvQ@mail.gmail.com>
 <OS7PR01MB11964C83E8E543743581250B0EAB3A@OS7PR01MB11964.jpnprd01.prod.outlook.com>
 <TYCPR01MB1131647E781856780072FBAE9E8B0A@TYCPR01MB11316.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="neA8os+yp0GAmUQd"
Content-Disposition: inline
In-Reply-To: 
 <TYCPR01MB1131647E781856780072FBAE9E8B0A@TYCPR01MB11316.jpnprd01.prod.outlook.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aVHhew6_9O7rD4RR%40paquier.xyz>
Precedence: bulk


--neA8os+yp0GAmUQd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 26, 2025 at 10:17:27AM +0000, Ryo Matsumura (Fujitsu) wrote:
> +1 to Allow-background-workers-to-be-terminated
>=20
> The result is same, so I think it's better to prioritize compatibility.
>=20
> PGWORKER_PROTECTED would be used in scenarios like the following:
> Existing features are probably not designed to be forcibly stopped.
> Therefore, all existing features should have PROTECTED applied to them.
> Most newly implemented features will also have PROTECTED applied because =
it requires less thought and is safer.
> Only considerate developers of features that can easily guarantee safety =
would adopt the default.

We could design things so as we have a second flag to force a bgworker
to be non-interuptible, then we could force that either the
interruptible flag or the non-interruptible flag should be set.  What
is mentioned as a problem is that 0 implies that the non-interruptible
is enforced.  I don't think that we would have much to gain by doing
that, as it would just lead to extension breakages that we can avoid.

> In conclusion, this is no different from BGWORKER_INTERRUPTABLE.
> Therefore, I think it's better to prioritize compatibility.

Looking finally at the patch, I like the simplicity of what you are
doing here.

+      Requires both <literal>BGWORKER_SHMEM_ACCESS</literal> and
+      <literal>BGWORKER_BACKEND_DATABASE_CONNECTION</literal>.

SanityCheckBackgroundWorker() does not enforce this check when
registering a bgworker.  But shouldn't we do so when the interruptible
flag is set?

+void
+TerminateInterruptableBgWorkersByDbOid(Oid databaseId)

Fine by me to aim for simplicity with this interface, discarding my
previous fancy comment about the extensibility we could do here.
Matsumura-san and I have also discussed a bit that offline at the last
JPUG, where I said that I'm OK with simpler at the end.

One issue with the test as written, as of run_db_command(), is that we
make sure that a worker is stopped by scanning the output of the logs.
This approach may detect incorrect patterns, unfortunately.  For
example, if the termination logic has a bug it may be possible that
the worker found as terminated is the first one created by the test,
which we expect to always run.  While the log is mandatory to have, I
have a suggestion to make that even better: let's keep track in
run_db_command() of the PIDs of the worker processes we expect to
exist after running each database command, then make sure that the
list of PIDs match with what we expect.  This is a bit simpler in the
case of this test as we only expect one matching PID.
--
Michael

--neA8os+yp0GAmUQd
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmlR4XsACgkQnvQgOdby
QH3rhg/9HTmEfVlHGBq1UlX0tEIOBOBWqj96edwbILTPiCmH1Ekp1Cl46KNeEnxi
NNwDGCdaRSJyuh1yNuW1U2dQJTF8tjYDUqkFbhWrH0DkV3c4rWwES4f7RYLB0IhQ
9ynt/+8PgsDDUDD/5rSQV2zQtrRlYOrbDw17JF+CcLWzoH5NGr8tbEz+ngPlWKnF
34YGt+L+9Orr6wmdk/rBwr1QfRYBzo8kEB2vaL+8NiPYF0l2iXqn+9eeJiktYMNL
I0XXBQld0l3O6uRMWQcOr77hO7nMDSsezxC1kakVfzdQ0YtYDU3hcT0NYy5hYirI
kFf7HmEjb3lD3ymOOf7CAt9x2zBhiGSnEbOEK7Lmf5Wi6nhM+JjL1/hV24p9W32R
WWDSh6Bwzzw3Fp1IpYXTCi5ElvPhlEReX7CgOfADi2/MTotL491M6Yrtoo+LDNGU
KDD4RDRX3emhG/DZeef5/sShSCfAWS2C0NyHAll/Iu77lXUXNHaP6AVQG/gDAbeD
tWQgRpf2SJPlQFvX8SumXG0vO6zym+W7bkn6vXZly0LpSrIl5JJWPs0ac+DeiGwl
4fQH26Vq/qjDpX1y/EadAy1swOHR5nMdR0mAX1N2vWwoSRjAkXx+WK/kuff9GwzD
oNH29omiOUeShmzo3DZVrJXVvROl9TDK/I0ezUeKqZCxGYyyqG0=
=geMu
-----END PGP SIGNATURE-----

--neA8os+yp0GAmUQd--