Re: Support worker_spi to execute the function dynamically.
Masahiro Ikeda <ikedamsh@oss.nttdata.com>
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
pgsql-hackers@lists.postgresql.org
Date: 2023-07-20T09:08:04Z
Lists: pgsql-hackers
Attachments
- v2-0001-Support-worker_spi-to-execute-the-function-dynamical.patch (text/x-diff) patch v2-0001
Hi, On 2023-07-20 13:50, Bharath Rupireddy wrote: > On Thu, Jul 20, 2023 at 10:09 AM Michael Paquier <michael@paquier.xyz> > wrote: >> >> On Thu, Jul 20, 2023 at 09:43:37AM +0530, Bharath Rupireddy wrote: >> > +1. However, a comment above helps one to understand why some GUCs are >> > defined before if (!process_shared_preload_libraries_in_progress). As >> > this is an example extension, it will help understand the reasoning >> > better. I know we will it in the commit message, but a direct comment >> > helps: >> > >> > /* >> > * Note that this GUC is defined irrespective of worker_spi shared library >> > * presence in shared_preload_libraries. It's possible to create the >> > * worker_spi extension and use functions without it being specified in >> > * shared_preload_libraries. If we return from here without defining this >> > * GUC, the dynamic workers launched by worker_spi_launch() will keep >> > * crashing and restarting. >> > */ >> >> WFM to be more talkative here and document things, but I don't think >> that's it. How about a simple "These GUCs are defined even if this >> library is not loaded with shared_preload_libraries, for >> worker_spi_launch()." > > LGTM. 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. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Commits
-
worker_spi: Fix race condition in newly-added TAP tests
- b68e356a680e 17.0 landed
-
worker_spi: Switch to TAP tests
- 320c311fda91 17.0 landed
-
worker_spi: Use term "dynamic" for bgworkers launched with worker_spi_launch()
- d9eb92c7b122 17.0 landed
-
Fix worker_spi when launching workers without shared_preload_libraries
- 97ff8dd02ca7 17.0 landed