On 31/03/17 15:42, Robert Haas wrote: > On Tue, Mar 28, 2017 at 7:39 PM, Petr Jelinek > <petr.jeli...@2ndquadrant.com> wrote: >> Sigh, forgot git add for the docs, so one more try... > > + if (strncmp(worker->bgw_library_name, "postgres", 8) != 0) > + return NULL; > > I think that's not right. You don't want to match postgresshdkgjsdglkjs. >
Right, changed to BGW_MAXLEN. > Aside from that, these look good to me. > Cool. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
From 74b9f0d7063071dd443fa820984fba477f4446cc Mon Sep 17 00:00:00 2001 From: Petr Jelinek <pjmodos@pjmodos.net> Date: Tue, 28 Mar 2017 23:24:34 +0200 Subject: [PATCH] Use library_name/function_name for loading main entry point of internal bgworkers --- src/backend/access/transam/parallel.c | 7 +-- src/backend/postmaster/bgworker.c | 76 ++++++++++++++++++++++++++------ src/include/access/parallel.h | 2 + src/test/modules/worker_spi/worker_spi.c | 6 ++- 4 files changed, 73 insertions(+), 18 deletions(-) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index cde0ed3..d2bed72 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -109,7 +109,6 @@ static dlist_head pcxt_list = DLIST_STATIC_INIT(pcxt_list); /* Private functions. */ static void HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg); static void ParallelExtensionTrampoline(dsm_segment *seg, shm_toc *toc); -static void ParallelWorkerMain(Datum main_arg); static void WaitForParallelWorkersToExit(ParallelContext *pcxt); @@ -456,7 +455,9 @@ LaunchParallelWorkers(ParallelContext *pcxt) BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION; worker.bgw_start_time = BgWorkerStart_ConsistentState; worker.bgw_restart_time = BGW_NEVER_RESTART; - worker.bgw_main = ParallelWorkerMain; + worker.bgw_main = NULL; + sprintf(worker.bgw_library_name, "postgres"); + sprintf(worker.bgw_function_name, "ParallelWorkerMain"); worker.bgw_main_arg = UInt32GetDatum(dsm_segment_handle(pcxt->seg)); worker.bgw_notify_pid = MyProcPid; memset(&worker.bgw_extra, 0, BGW_EXTRALEN); @@ -928,7 +929,7 @@ AtEOXact_Parallel(bool isCommit) /* * Main entrypoint for parallel workers. */ -static void +void ParallelWorkerMain(Datum main_arg) { dsm_segment *seg; diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index 52bc4e9..5ea5abf 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -16,6 +16,7 @@ #include "miscadmin.h" #include "libpq/pqsignal.h" +#include "access/parallel.h" #include "postmaster/bgworker_internals.h" #include "postmaster/postmaster.h" #include "storage/barrier.h" @@ -94,6 +95,25 @@ struct BackgroundWorkerHandle static BackgroundWorkerArray *BackgroundWorkerData; /* + * List of internal background workers. These are used for mapping the + * function name to actual function when building with EXEC_BACKEND and also + * to allow these to be loaded outside of shared_preload_libraries. + */ +typedef struct InternalBGWorkerMain +{ + char *bgw_function_name; + bgworker_main_type bgw_main; +} InternalBGWorkerMain; + +static const InternalBGWorkerMain InternalBGWorkers[] = { + {"ParallelWorkerMain", ParallelWorkerMain}, + /* Dummy entry marking end of the array. */ + {NULL, NULL} +}; + +static bgworker_main_type GetInternalBgWorkerMain(BackgroundWorker *worker); + +/* * Calculate shared memory needed. */ Size @@ -695,22 +715,27 @@ StartBackgroundWorker(void) #endif } + /* For internal workers set the entry point to known function address. */ + entrypt = GetInternalBgWorkerMain(worker); + /* - * If bgw_main is set, we use that value as the initial entrypoint. - * However, if the library containing the entrypoint wasn't loaded at - * postmaster startup time, passing it as a direct function pointer is not - * possible. To work around that, we allow callers for whom a function - * pointer is not available to pass a library name (which will be loaded, - * if necessary) and a function name (which will be looked up in the named + * Otherwise, if bgw_main is set, we use that value as the initial + * entrypoint. This does not work well EXEC_BACKEND outside Windows but + * we keep the logic for backwards compatibility. In other cases use + * the entry point specified by library name (which will be loaded, if + * necessary) and a function name (which will be looked up in the named * library). */ - 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); + if (entrypt == NULL) + { + 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); + } /* * Note that in normal processes, we would call InitPostgres here. For a @@ -1050,3 +1075,28 @@ TerminateBackgroundWorker(BackgroundWorkerHandle *handle) if (signal_postmaster) SendPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE); } + +/* + * Search the known internal worker array and return its main function + * pointer if found. + * + * Returns NULL if not known internal worker. + */ +static bgworker_main_type +GetInternalBgWorkerMain(BackgroundWorker *worker) +{ + int i; + + /* Internal workers always have to use postgres as library name. */ + if (strncmp(worker->bgw_library_name, "postgres", BGW_MAXLEN) != 0) + return NULL; + + for (i = 0; InternalBGWorkers[i].bgw_function_name; i++) + { + if (strncmp(InternalBGWorkers[i].bgw_function_name, + worker->bgw_function_name, BGW_MAXLEN) == 0) + return InternalBGWorkers[i].bgw_main; + } + + return NULL; +} diff --git a/src/include/access/parallel.h b/src/include/access/parallel.h index 2f8f36f..61dbfda 100644 --- a/src/include/access/parallel.h +++ b/src/include/access/parallel.h @@ -67,4 +67,6 @@ extern void AtEOXact_Parallel(bool isCommit); extern void AtEOSubXact_Parallel(bool isCommit, SubTransactionId mySubId); extern void ParallelWorkerReportLastRecEnd(XLogRecPtr last_xlog_end); +extern void ParallelWorkerMain(Datum main_arg); + #endif /* PARALLEL_H */ diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c index 7c9a3eb..3711fba 100644 --- a/src/test/modules/worker_spi/worker_spi.c +++ b/src/test/modules/worker_spi/worker_spi.c @@ -346,7 +346,9 @@ _PG_init(void) BGWORKER_BACKEND_DATABASE_CONNECTION; worker.bgw_start_time = BgWorkerStart_RecoveryFinished; worker.bgw_restart_time = BGW_NEVER_RESTART; - worker.bgw_main = worker_spi_main; + worker.bgw_main = NULL; + sprintf(worker.bgw_library_name, "worker_spi"); + sprintf(worker.bgw_function_name, "worker_spi_main"); worker.bgw_notify_pid = 0; /* @@ -377,7 +379,7 @@ worker_spi_launch(PG_FUNCTION_ARGS) BGWORKER_BACKEND_DATABASE_CONNECTION; worker.bgw_start_time = BgWorkerStart_RecoveryFinished; worker.bgw_restart_time = BGW_NEVER_RESTART; - worker.bgw_main = NULL; /* new worker might not have library loaded */ + worker.bgw_main = NULL; sprintf(worker.bgw_library_name, "worker_spi"); sprintf(worker.bgw_function_name, "worker_spi_main"); snprintf(worker.bgw_name, BGW_MAXLEN, "worker %d", i); -- 2.7.4
From c16978bfb8d8beac7fad3833b7fc0b03e1377d2d Mon Sep 17 00:00:00 2001 From: Petr Jelinek <pjmodos@pjmodos.net> Date: Tue, 28 Mar 2017 04:21:46 +0200 Subject: [PATCH] Use library_name/function_name for loading main entry point of internal bgworkers Also remove bgw_main as it's not safe to use even for shared_preload_libraries. --- doc/src/sgml/bgworker.sgml | 36 ++++--------- src/backend/access/transam/parallel.c | 6 +-- src/backend/postmaster/bgworker.c | 81 +++++++++++++++++++----------- src/backend/replication/logical/launcher.c | 6 ++- src/include/access/parallel.h | 2 + src/include/postmaster/bgworker.h | 5 +- src/test/modules/test_shm_mq/setup.c | 1 - src/test/modules/worker_spi/worker_spi.c | 4 +- 8 files changed, 75 insertions(+), 66 deletions(-) diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml index 07f9f10..33cfdaf 100644 --- a/doc/src/sgml/bgworker.sgml +++ b/doc/src/sgml/bgworker.sgml @@ -54,9 +54,8 @@ typedef struct BackgroundWorker int bgw_flags; BgWorkerStartTime bgw_start_time; int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */ - bgworker_main_type bgw_main; - char bgw_library_name[BGW_MAXLEN]; /* only if bgw_main is NULL */ - char bgw_function_name[BGW_MAXLEN]; /* only if bgw_main is NULL */ + char bgw_library_name[BGW_MAXLEN]; + char bgw_function_name[BGW_MAXLEN]; Datum bgw_main_arg; char bgw_extra[BGW_EXTRALEN]; int bgw_notify_pid; @@ -131,26 +130,12 @@ typedef struct BackgroundWorker </para> <para> - <structfield>bgw_main</structfield> is a pointer to the function to run when - the process is started. This field can only safely be used to launch - functions within the core server, because shared libraries may be loaded - at different starting addresses in different backend processes. This will - happen on all platforms when the library is loaded using any mechanism - other than <xref linkend="guc-shared-preload-libraries">. Even when that - mechanism is used, address space layout variations will still occur on - Windows, and when <literal>EXEC_BACKEND</> is used. Therefore, most users - of this API should set this field to NULL. If it is non-NULL, it takes - precedence over <structfield>bgw_library_name</> and - <structfield>bgw_function_name</>. - </para> - - <para> <structfield>bgw_library_name</structfield> is the name of a library in which the initial entry point for the background worker should be sought. The named library will be dynamically loaded by the worker process and <structfield>bgw_function_name</structfield> will be used to identify the - function to be called. If loading a function from the core code, - <structfield>bgw_main</> should be set instead. + function to be called. If loading a function from the core code, this must + be set to "postgres". </para> <para> @@ -161,13 +146,10 @@ typedef struct BackgroundWorker <para> <structfield>bgw_main_arg</structfield> is the <type>Datum</> argument - to the background worker main function. Regardless of whether that - function is specified via <structfield>bgw_main</> or via the combination - of <function>bgw_library_name</> and <function>bgw_function_name</>, - this main function should take a single argument of type <type>Datum</> - and return <type>void</>. <structfield>bgw_main_arg</structfield> will be - passed as the argument. In addition, the global variable - <literal>MyBgworkerEntry</literal> + to the background worker main function. This main function should take a + single argument of type <type>Datum</> and return <type>void</>. + <structfield>bgw_main_arg</structfield> will be passed as the argument. + In addition, the global variable <literal>MyBgworkerEntry</literal> points to a copy of the <structname>BackgroundWorker</structname> structure passed at registration time; the worker may find it helpful to examine this structure. @@ -215,7 +197,7 @@ typedef struct BackgroundWorker <para> Signals are initially blocked when control reaches the - <structfield>bgw_main</> function, and must be unblocked by it; this is to + background workers' main function, and must be unblocked by it; this is to allow the process to customize its signal handlers, if necessary. Signals can be unblocked in the new process by calling <function>BackgroundWorkerUnblockSignals</> and blocked by calling diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 3e0ee87..b3d3853 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -110,7 +110,6 @@ static dlist_head pcxt_list = DLIST_STATIC_INIT(pcxt_list); /* Private functions. */ static void HandleParallelMessage(ParallelContext *pcxt, int i, StringInfo msg); static void ParallelExtensionTrampoline(dsm_segment *seg, shm_toc *toc); -static void ParallelWorkerMain(Datum main_arg); static void WaitForParallelWorkersToExit(ParallelContext *pcxt); @@ -458,7 +457,8 @@ LaunchParallelWorkers(ParallelContext *pcxt) | BGWORKER_CLASS_PARALLEL; worker.bgw_start_time = BgWorkerStart_ConsistentState; worker.bgw_restart_time = BGW_NEVER_RESTART; - worker.bgw_main = ParallelWorkerMain; + sprintf(worker.bgw_library_name, "postgres"); + sprintf(worker.bgw_function_name, "ParallelWorkerMain"); worker.bgw_main_arg = UInt32GetDatum(dsm_segment_handle(pcxt->seg)); worker.bgw_notify_pid = MyProcPid; memset(&worker.bgw_extra, 0, BGW_EXTRALEN); @@ -931,7 +931,7 @@ AtEOXact_Parallel(bool isCommit) /* * Main entrypoint for parallel workers. */ -static void +void ParallelWorkerMain(Datum main_arg) { dsm_segment *seg; diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index 10e0f88..0823317 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -15,12 +15,14 @@ #include <unistd.h> #include "libpq/pqsignal.h" +#include "access/parallel.h" #include "miscadmin.h" #include "pgstat.h" #include "port/atomics.h" #include "postmaster/bgworker_internals.h" #include "postmaster/postmaster.h" #include "replication/logicallauncher.h" +#include "replication/logicalworker.h" #include "storage/dsm.h" #include "storage/ipc.h" #include "storage/latch.h" @@ -109,14 +111,26 @@ struct BackgroundWorkerHandle static BackgroundWorkerArray *BackgroundWorkerData; /* - * List of workers that are allowed to be started outside of - * shared_preload_libraries. + * List of internal background workers. These are used for mapping the + * function name to actual function when building with EXEC_BACKEND and also + * to allow these to be loaded outside of shared_preload_libraries. */ -static const bgworker_main_type InternalBGWorkers[] = { - ApplyLauncherMain, - NULL +typedef struct InternalBGWorkerMain +{ + char *bgw_function_name; + bgworker_main_type bgw_main; +} InternalBGWorkerMain; + +static const InternalBGWorkerMain InternalBGWorkers[] = { + {"ParallelWorkerMain", ParallelWorkerMain}, + {"ApplyLauncherMain", ApplyLauncherMain}, + {"ApplyWorkerMain", ApplyWorkerMain}, + /* Dummy entry marking end of the array. */ + {NULL, NULL} }; +static bgworker_main_type GetInternalBgWorkerMain(BackgroundWorker *worker); + /* * Calculate shared memory needed. */ @@ -341,7 +355,6 @@ BackgroundWorkerStateChange(void) rw->rw_worker.bgw_flags = slot->worker.bgw_flags; rw->rw_worker.bgw_start_time = slot->worker.bgw_start_time; rw->rw_worker.bgw_restart_time = slot->worker.bgw_restart_time; - rw->rw_worker.bgw_main = slot->worker.bgw_main; rw->rw_worker.bgw_main_arg = slot->worker.bgw_main_arg; memcpy(rw->rw_worker.bgw_extra, slot->worker.bgw_extra, BGW_EXTRALEN); @@ -763,17 +776,14 @@ StartBackgroundWorker(void) } /* - * If bgw_main is set, we use that value as the initial entrypoint. - * However, if the library containing the entrypoint wasn't loaded at - * postmaster startup time, passing it as a direct function pointer is not - * possible. To work around that, we allow callers for whom a function - * pointer is not available to pass a library name (which will be loaded, - * if necessary) and a function name (which will be looked up in the named - * library). + * For internal workers set the entry point to known function address. + * Otherwise use the entry point specified by library name (which will + * be loaded, if necessary) and a function name (which will be looked up + * in the named library). */ - if (worker->bgw_main != NULL) - entrypt = worker->bgw_main; - else + entrypt = GetInternalBgWorkerMain(worker); + + if (entrypt == NULL) entrypt = (bgworker_main_type) load_external_function(worker->bgw_library_name, worker->bgw_function_name, @@ -806,23 +816,13 @@ RegisterBackgroundWorker(BackgroundWorker *worker) { RegisteredBgWorker *rw; static int numworkers = 0; - bool internal = false; - int i; if (!IsUnderPostmaster) ereport(DEBUG1, (errmsg("registering background worker \"%s\"", worker->bgw_name))); - for (i = 0; InternalBGWorkers[i]; i++) - { - if (worker->bgw_main == InternalBGWorkers[i]) - { - internal = true; - break; - } - } - - if (!process_shared_preload_libraries_in_progress && !internal) + if (!process_shared_preload_libraries_in_progress && + GetInternalBgWorkerMain(worker) == NULL) { if (!IsUnderPostmaster) ereport(LOG, @@ -1152,3 +1152,28 @@ TerminateBackgroundWorker(BackgroundWorkerHandle *handle) if (signal_postmaster) SendPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE); } + +/* + * Search the known internal worker array and return its main function + * pointer if found. + * + * Returns NULL if not known internal worker. + */ +static bgworker_main_type +GetInternalBgWorkerMain(BackgroundWorker *worker) +{ + int i; + + /* Internal workers always have to use postgres as library name. */ + if (strncmp(worker->bgw_library_name, "postgres", BGW_MAXLEN) != 0) + return NULL; + + for (i = 0; InternalBGWorkers[i].bgw_function_name; i++) + { + if (strncmp(InternalBGWorkers[i].bgw_function_name, + worker->bgw_function_name, BGW_MAXLEN) == 0) + return InternalBGWorkers[i].bgw_main; + } + + return NULL; +} diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c index 0d3ec27..226a168 100644 --- a/src/backend/replication/logical/launcher.c +++ b/src/backend/replication/logical/launcher.c @@ -295,7 +295,8 @@ logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid, bgw.bgw_flags = BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION; bgw.bgw_start_time = BgWorkerStart_RecoveryFinished; - bgw.bgw_main = ApplyWorkerMain; + snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres"); + snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ApplyWorkerMain"); if (OidIsValid(relid)) snprintf(bgw.bgw_name, BGW_MAXLEN, "logical replication worker for subscription %u sync %u", subid, relid); @@ -553,7 +554,8 @@ ApplyLauncherRegister(void) bgw.bgw_flags = BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION; bgw.bgw_start_time = BgWorkerStart_RecoveryFinished; - bgw.bgw_main = ApplyLauncherMain; + snprintf(bgw.bgw_library_name, BGW_MAXLEN, "postgres"); + snprintf(bgw.bgw_function_name, BGW_MAXLEN, "ApplyLauncherMain"); snprintf(bgw.bgw_name, BGW_MAXLEN, "logical replication launcher"); bgw.bgw_restart_time = 5; diff --git a/src/include/access/parallel.h b/src/include/access/parallel.h index 96ce480..5065a38 100644 --- a/src/include/access/parallel.h +++ b/src/include/access/parallel.h @@ -67,4 +67,6 @@ extern void AtEOXact_Parallel(bool isCommit); extern void AtEOSubXact_Parallel(bool isCommit, SubTransactionId mySubId); extern void ParallelWorkerReportLastRecEnd(XLogRecPtr last_xlog_end); +extern void ParallelWorkerMain(Datum main_arg); + #endif /* PARALLEL_H */ diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h index 128e92c..51a5978 100644 --- a/src/include/postmaster/bgworker.h +++ b/src/include/postmaster/bgworker.h @@ -91,9 +91,8 @@ typedef struct BackgroundWorker int bgw_flags; BgWorkerStartTime bgw_start_time; int bgw_restart_time; /* in seconds, or BGW_NEVER_RESTART */ - bgworker_main_type bgw_main; - char bgw_library_name[BGW_MAXLEN]; /* only if bgw_main is NULL */ - char bgw_function_name[BGW_MAXLEN]; /* only if bgw_main is NULL */ + char bgw_library_name[BGW_MAXLEN]; + char bgw_function_name[BGW_MAXLEN]; Datum bgw_main_arg; char bgw_extra[BGW_EXTRALEN]; pid_t bgw_notify_pid; /* SIGUSR1 this backend on start/stop */ diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c index e3c024e..319a67f 100644 --- a/src/test/modules/test_shm_mq/setup.c +++ b/src/test/modules/test_shm_mq/setup.c @@ -216,7 +216,6 @@ setup_background_workers(int nworkers, dsm_segment *seg) worker.bgw_flags = BGWORKER_SHMEM_ACCESS; worker.bgw_start_time = BgWorkerStart_ConsistentState; worker.bgw_restart_time = BGW_NEVER_RESTART; - worker.bgw_main = NULL; /* new worker might not have library loaded */ sprintf(worker.bgw_library_name, "test_shm_mq"); sprintf(worker.bgw_function_name, "test_shm_mq_main"); snprintf(worker.bgw_name, BGW_MAXLEN, "test_shm_mq"); diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c index 72ab846..421ec76 100644 --- a/src/test/modules/worker_spi/worker_spi.c +++ b/src/test/modules/worker_spi/worker_spi.c @@ -347,7 +347,8 @@ _PG_init(void) BGWORKER_BACKEND_DATABASE_CONNECTION; worker.bgw_start_time = BgWorkerStart_RecoveryFinished; worker.bgw_restart_time = BGW_NEVER_RESTART; - worker.bgw_main = worker_spi_main; + sprintf(worker.bgw_library_name, "worker_spi"); + sprintf(worker.bgw_function_name, "worker_spi_main"); worker.bgw_notify_pid = 0; /* @@ -378,7 +379,6 @@ worker_spi_launch(PG_FUNCTION_ARGS) BGWORKER_BACKEND_DATABASE_CONNECTION; worker.bgw_start_time = BgWorkerStart_RecoveryFinished; worker.bgw_restart_time = BGW_NEVER_RESTART; - worker.bgw_main = NULL; /* new worker might not have library loaded */ sprintf(worker.bgw_library_name, "worker_spi"); sprintf(worker.bgw_function_name, "worker_spi_main"); snprintf(worker.bgw_name, BGW_MAXLEN, "worker %d", i); -- 2.7.4
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers