On Thu, Jul 20, 2023 at 2:38 PM Masahiro Ikeda <ikeda...@oss.nttdata.com> wrote: > > Thanks for discussing about the patch. I updated the patch from your > comments > * v2-0001-Support-worker_spi-to-execute-the-function-dynamical.patch > > I found another thing to be changed better. Though the tests was assumed > "shared_preload_libraries = worker_spi", the background workers failed > to > be launched in initialized phase because the database is not created > yet. > > ``` > # make check # in src/test/modules/worker_spi > # cat log/postmaster.log # in src/test/modules/worker_spi/ > 2023-07-20 17:58:47.958 JST worker_spi[853620] FATAL: database > "contrib_regression" does not exist > 2023-07-20 17:58:47.958 JST worker_spi[853621] FATAL: database > "contrib_regression" does not exist > 2023-07-20 17:58:47.959 JST postmaster[853612] LOG: background worker > "worker_spi" (PID 853620) exited with exit code 1 > 2023-07-20 17:58:47.959 JST postmaster[853612] LOG: background worker > "worker_spi" (PID 853621) exited with exit code 1 > ``` > > It's better to remove "shared_preload_libraries = worker_spi" from the > test configuration. I misunderstood that two background workers would > be launched and waiting at the start of the test.
I don't think that change is correct. The worker_spi essentially shows how to start bg workers with RegisterBackgroundWorker and dynamic bg workers with RegisterDynamicBackgroundWorker. If shared_preload_libraries = worker_spi not specified in there, you will miss to start RegisterBackgroundWorkers. Is giving an initidb time database name to worker_spi.database work there? If the database for bg workers doesn't exist, changing bgw_restart_time from BGW_NEVER_RESTART to say 1 will help to see bg workers coming up eventually. I think it's worth adding test cases for the expected number of bg workers (after creating worker_spi extension) and dynamic bg workers (after calling worker_spi_launch()). Also, to distinguish bg workers and dynamic bg workers, you can change bgw_type in worker_spi_launch to "worker_spi dynamic worker". - /* get the configuration */ + /* Get the configuration */ - /* set up common data for all our workers */ + /* Set up common data for all our workers */ These unrelated changes better be there as-is. Because, the postgres code has both commenting styles /* Get .... */ or /* get ....*/, IOW, single line comments starting with both uppercase and lowercase. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
