Thread: Server crash when using bgw_main for a dynamic bgworker
Hi all,
While playing a bit with background workers (commit 527ea66), I found that setting bgw_main for a dynamic bgworker, as well as bgw_library_name and bgw_library_name, crashes to server if the library defining the function defined in bgw_main is not loaded. In order to reproduce that, for example simply change bgw_main from NULL to worker_spi_main in worker_spi_launch:worker_spi.c and do not set shared_preload_libraries with worker_spi. Then connect to a server having this modified worker_spi installed and do the following:
postgres=# show shared_preload_libraries ;
shared_preload_libraries
--------------------------
(1 row)
postgres=# create extension worker_spi;
CREATE EXTENSION
postgres=# select worker_spi_launch(1);
worker_spi_launch
-------------------
t
(1 row)
postgres=# select worker_spi_launch(2);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.
By looking at the code, priority is given to bgw_main...
if (worker->bgw_main != NULL)
entrypt = worker->bgw_main;
else
entrypt = (bgworker_main_type)
load_external_function(worker->bgw_library_name,
worker->bgw_function_name,
true, NULL);
Wouldn't be clearer for the user to add a new flag in BackgroundWorker:bgworker.h to define a class of bgworker? Or at least specify clearly in the docs just to never set bgw_main if the library is not loaded previously using for example shared_preload_libraries?
While playing a bit with background workers (commit 527ea66), I found that setting bgw_main for a dynamic bgworker, as well as bgw_library_name and bgw_library_name, crashes to server if the library defining the function defined in bgw_main is not loaded. In order to reproduce that, for example simply change bgw_main from NULL to worker_spi_main in worker_spi_launch:worker_spi.c and do not set shared_preload_libraries with worker_spi. Then connect to a server having this modified worker_spi installed and do the following:
postgres=# show shared_preload_libraries ;
shared_preload_libraries
--------------------------
(1 row)
postgres=# create extension worker_spi;
CREATE EXTENSION
postgres=# select worker_spi_launch(1);
worker_spi_launch
-------------------
t
(1 row)
postgres=# select worker_spi_launch(2);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.
By looking at the code, priority is given to bgw_main...
if (worker->bgw_main != NULL)
entrypt = worker->bgw_main;
else
entrypt = (bgworker_main_type)
load_external_function(worker->bgw_library_name,
worker->bgw_function_name,
true, NULL);
Wouldn't be clearer for the user to add a new flag in BackgroundWorker:bgworker.h to define a class of bgworker? Or at least specify clearly in the docs just to never set bgw_main if the library is not loaded previously using for example shared_preload_libraries?
Opinions?
Regards,
--
Michael
On Mon, Aug 12, 2013 at 1:26 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > While playing a bit with background workers (commit 527ea66), I found that > setting bgw_main for a dynamic bgworker, as well as bgw_library_name and > bgw_library_name, crashes to server if the library defining the function > defined in bgw_main is not loaded. Yep. As the documentation says: "bgw_main may be NULL; in that case, bgw_library_name and bgw_function_name will be used to determine the entrypoint. This is useful for background workers launched after postmaster startup, where the postmaster does not have the requisite library loaded." http://www.postgresql.org/docs/devel/static/bgworker.html > Wouldn't be clearer for the user to add a new flag in > BackgroundWorker:bgworker.h to define a class of bgworker? Or at least > specify clearly in the docs just to never set bgw_main if the library is not > loaded previously using for example shared_preload_libraries? I don't think adding a flag really eliminates any possibility for user confusion that doesn't already exist as things stand. However, changing the documentation might make sense. The key point is that if the function must be loaded into both the registering backend and the worker backend, and it must also be loaded into both backends *at the same address*. Anything that's loaded later than shared_preload_libraries isn't likely to be soon enough, and even if you do load the library at shared_preload_libraries time, I'm not clear that it's going to work reliably in an EXEC_BACKEND environment.What if the OS uses ASLR and puts the shared libraryat different addresses in different child processes? It'll crash, that's what. It's almost tempting to tell people "don't use bgw_main", but I think that might be going overboard. This is expert-level hackery; experts don't usually like being told what to do without being told why they must. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company