Thread

RE: [PROPOSAL] Termination of Background Workers for ALTER/DROP DATABASE

Aya Iwata (Fujitsu) <iwata.aya@fujitsu.com> — 2025-10-16T15:04:09Z

Hi Chao-san, Michael san

Thank you for your comments! To accept your comment, I updated patch to v0008.


> From: Chao Li <li.evan.chao@gmail.com> 
> Sent: Wednesday, October 15, 2025 12:37 PM
...
> By searching for “ByOid”, we can get some existing examples:
>
> ObjectAddress
> RefreshMatViewByOid(Oid matviewOid, bool is_create, bool skipData,
>                     bool concurrent, const char *queryString,
>                     QueryCompletion *qc)
>
> The function name clearly tells refresh MatView by Oid, so the oid in parameter is an old of mat view.
>
> ResultRelInfo *
> ExecLookupResultRelByOid(ModifyTableState *node, Oid resultoid,
>                          bool missing_ok, bool update_cache)
>
> The function name indicates ResultRel, so the oid is a result oid.
>
> AccessMethodInfo *
> findAccessMethodByOid(Oid oid)
>
> The function name tells to find access method, the the oid is an access method’s OID.
>
> You can find more …
>
> But in this patch, the function name only indeeds “terminate background workers”, while the oid is a database oid. Maybe we can rename the 
> function to “TerminateDatabaseBgWorkersByOid()”.

Thank you. I changed the function name to "'TerminateBgWorkersByDbOid".
I prefer this name because there are not official terminology "Database background worker" and it's shorter.

> -----Original Message-----
> From: Michael Paquier <michael@paquier.xyz>
> Sent: Thursday, October 16, 2025 12:55 PM
...
> On Wed, Oct 15, 2025 at 02:48:43AM +0000, Aya Iwata (Fujitsu) wrote:
 
> + * Exit the bgworker when its database is dropped, renamed, moved to a
> + * different tablespace, or used as a template for CREATE DATABASE.
> 
> I don't think that we need to list all these operations in details
> here. We could just say "if its database is involved in a CREATE,
> ALTER or DROP database command".  The docs should provide these
> details, of course.

Thank you. I fixed this .h file comment.

> 
> +#define BGWORKER_EXIT_AT_DATABASE_CHANGE       0x0004
> 
> Flag name works here.

Sorry, I cannot follow. Please tell me more details about this comment.

> # XXX This spends more than 5 seconds because the backend retries counting
> # number of connecting processes 50 times. See CountOtherDBBackends().
> 
> And that's annoying.  Let's activate what I call the cheat mode for
> this one: an injection point that, if defined, enforces a lower number
> of tries when we loop over the workers to stop.  That would make the
> test much faster when using a worker that should not be stopped,
> without impacting the coverage.

I tried to implement your idea. Thanks Kuroda-san to help it.

> I suspect that your new test 002_worker_terminate.pl has a race
> condition in run_db_command(): are you sure that the bgworker has
> enough time to be reported as stopped in the server logs once
> safe_psql() finishes to run the database command given by the caller?
> On very slow and/or loaded machines, particularly, that could hurt the
> stability.  It seems to me that this should use a wait_for_log()
> instead of a log_contains(), waiting for the worker to be reported as
> stopped depending on the command executed.

I fixed this test to use wait_for_log() instead of log_contains().

> Shouldn't this test also check that worker 0 (the one that does not
> have the flag set) is still running at the end of the test?  I assume
> that querying pg_stat_activity would be enough at the end of the
> script.

Added. I cannot find a good way to clarify the worker is "worker 0" from the pg_stat_activity, 
backend_type does not have the information. Thus I used the string "worker_spi dynamic" as the key.


Best regards,
Aya Iwata 
Fujitsu Limited