RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE - Mailing list pgsql-hackers

Hi,

I updated the patch to v0005.

> -----Original Message-----
> From: Peter Smith <smithpb2250@gmail.com>
> Sent: Thursday, October 9, 2025 7:18 AM

> src/backend/postmaster/bgworker.c
>
> 1.

> 1a.
> It's not clear to me what you were trying to convey by saying "unless
> slot has been used" in the comment. Maybe you meant "unless slot is
> not in use", but is that useful even to say? Anyway, the comment as-is
> seems incorrect.

I changed this comment. I use Kuroda-san's comment. Thank you. )
"Iterate through slots, looking for workers who connects to the given database."


> 1b.
> Sorry for wavering on this, but now that I see the resulting v4 code,
> I feel we don't really need any of those 'continues', and more if
> conditions can be combined. It becomes simpler. See if you agree.

I changed if condition code.

> src/backend/storage/ipc/procarray.c
>
> 2.

> I was wondering about this function name "CancelXXX" -- do you
> "cancel" a worker, or do you "terminate" it?
>
> Isn't it better to name this new function more like the
> existing/similar TerminateBackgroundWorker() function?
>
> E.g. consider the following:
>
> /*
>  * Terminate all background workers for this database, if
>  * they had requested it (BGWORKER_EXIT_AT_DATABASE_DROP).
>  */
> TerminateBackgroundWorkersForDB(databaseId);

I changed this name to "TerminateBackgroudWorkerByOid"


> -----Original Message-----
> From: Michael Paquier <michael@paquier.xyz>
> Sent: Thursday, October 9, 2025 12:34 PM

> > Sorry for posting many times. I noticed that CountOtherDBBackends() can be
> called
> > while creating the database. Should we also mention and test the case?
>
> How would you test that?  A bgworker would not be able to connect to
> the database that's being created.

> +my $basedir = $node->basedir();
> +my $tablespace = "$basedir/tablespace";
>
> We could use a temporary folder for the tablespaces.  I've always
> prefered this practice.  That's a bit, feel free to ignore this one,
> what you are doing is not wrong, either.

I use tempdir to create directory for the tablespace.

> +
> <term><literal>BGWORKER_EXIT_AT_DATABASE_DROP</literal></term>
>
> Perhaps BGWORKER_EXIT_AT_DATABASE_CHANGE?  DROP is incorrect, as
> the
> database could be renamed or moved, as well.
>
> + * Exit the bgworker when its database is dropped, renamed, or moved.
> + * Requires BGWORKER_SHMEM_ACCESS and
> BGWORKER_BACKEND_DATABASE_CONNECTION.
> + */
> +#define BGWORKER_EXIT_AT_DATABASE_DROP
> 0x0004
>
> We could enforce this rule with an elog(ERROR) or an assert, perhaps?

I started think it does not a strict condition.
If the worker does not connect to databases,
the BGWORKER_EXIT_AT_DATABASE_DROP(CHANGE) flag will be never checked.
Therefore, I have changed this comment as follows:
" No-op if BGWORKER_BACKEND_DATABASE_CONNECTION are not specified."

> @@ -407,7 +407,8 @@ worker_spi_launch(PG_FUNCTION_ARGS)
>      memset(&worker, 0, sizeof(worker));
>      worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
> -        BGWORKER_BACKEND_DATABASE_CONNECTION;
> +        BGWORKER_BACKEND_DATABASE_CONNECTION |
> +        BGWORKER_EXIT_AT_DATABASE_DROP;
>
> I would suggest to make this part optional, with an argument that
> uses a default value to false (as in "do-not-set the flag") that can
> be given by the callers of worker_spi_launch().  Let's also add one
> bgworker that's used in your series of tests, and check that it is
> *not* cancelled when the flag is not set.

I fixed this to added allow_termination flag and *not* canceled test.
However this test takes at least 5 sec. because of "for loop" in CountOtherDBBackends().

> +# Ensure the worker_spi dynamic worker is launched on the specified
> database
> +sub launch_bgworker
> +{
> +    my ($node, $database) = @_;
> +
> +    # Launch a background worker on the given database
> +    my $result = $node->safe_psql(
> +        $database, qq(
> +        SELECT worker_spi_launch(4, oid) IS NOT NULL
>
> I'd recommend to make the worker number an argument of this function,
> and also do things so as the log_contains() call is able to check that
> the worker with the matching number is loged, rather than rely on
> "worker_spi dynamic" for all the comparisons.  This is relevant to be
> able to mix multiple workers at the same time in the tests.

I fixed test code. Thank you for your advice.
It is better to used wait_for_log because it takes time for the log to output.
And we added test for "CREATE DATABASE TEMPLATE " command too.


Regards,
Aya Iwata
Fujitsu Limited