From: Petr Jelinek <petr.jelinek@2ndquadrant.com>
To: Robert Haas <robertmhaas@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Petr Jelinek <petr@2ndquadrant.com>
Cc: Peter Eisentraut <peter_e@gmx.net>
Subject: Re: logical replication launcher crash on buildfarm
Date: Tue, 28 Mar 2017 05:20:44 +0200
Message-ID: <e3c35f3a-426d-b1ca-7fe5-9241c2717013@2ndquadrant.com> (raw)
In-Reply-To: <CA+TgmobBTZ_eDbTmx-pfUndZDKPBAcj+r_3CQjEHFkp99kE2Yg@mail.gmail.com>
References: <20170316032833.2zxfnqe3sl3ktrbt@alap3.anarazel.de>
<20170316034246.3uurliezczmka6cn@alap3.anarazel.de>
<e56bda7f-9d29-6900-0f90-96be8219b930@2ndquadrant.com>
<20170316085322.crffknkgee5s6air@alap3.anarazel.de>
<368e64f6-ee9a-f09f-82b4-a33b61b28d36@2ndquadrant.com>
<20170327165023.t224nralrspk6udc@alap3.anarazel.de>
<CA+Tgmoa3C7=gVvmwvvKWT6+mkm=XGSDqF4bXu6=fohArafWHMQ@mail.gmail.com>
<52d78669-0523-4e60-a263-841c96210256@2ndquadrant.com>
<8941c9b1-f6e9-1056-39f2-7bc7336c4d00@2ndquadrant.com>
<20170328020422.i4srzzgq4zbzu2ci@alap3.anarazel.de>
<CA+TgmobBTZ_eDbTmx-pfUndZDKPBAcj+r_3CQjEHFkp99kE2Yg@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 28/03/17 04:46, Robert Haas wrote:
> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund <andres@anarazel.de> wrote:
>>> Btw now that I look at the code, I guess we'll want to get rid of
>>> bgw_main completely in HEAD given that we can't guarantee it will be
>>> valid even for shared_preload_library libraries. For older branches I
>>> would leave things as they are in this regard as there don't seem to be
>>> any immediate issue for standard binaries.
>>
>> As long as you fix it so culicidae is happy (in 9.6) ;). I think it's
>> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
>> code in place in < HEAD.
>
> I wasn't thinking of introducing bgw_builtin_id. My idea was just
> along the lines of
>
> if (bgw_library_name == NULL && bgw_function_name != NULL)
> {
> if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
> ParallelQueryMain(blah);
> else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
> LogicalReplicationMain(blah);
> }
>
> I think something like that is certainly better for the back-branches,
> because it doesn't cause an ABI break. But I think it would also be
> fine for master.
>
I had something slightly more complex like the attached in mind.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[binary/octet-stream] 0001-Remove-bgw_main-from-bgworker-and-always-use-library.patch (10.2K, ../e3c35f3a-426d-b1ca-7fe5-9241c2717013@2ndquadrant.com/2-0001-Remove-bgw_main-from-bgworker-and-always-use-library.patch)
download | inline diff:
From d548ff5467a6880122b5807add3d3c1febd8022e Mon Sep 17 00:00:00 2001
From: Petr Jelinek <pjmodos@pjmodos.net>
Date: Tue, 28 Mar 2017 04:21:46 +0200
Subject: [PATCH] Remove bgw_main from bgworker and always use library/function
for loading main entry point
---
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 | 1 -
src/test/modules/test_shm_mq/setup.c | 1 -
src/test/modules/worker_spi/worker_spi.c | 4 +-
7 files changed, 64 insertions(+), 37 deletions(-)
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.cindex 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.cindex 10e0f88..39a098c 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", 8) != 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.cindex 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.hindex 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.hindex 128e92c..7cc4c89 100644--- a/src/include/postmaster/bgworker.h+++ b/src/include/postmaster/bgworker.h@@ -91,7 +91,6 @@ 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 */
Datum bgw_main_arg;
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.cindex 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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: petr.jelinek@2ndquadrant.com, robertmhaas@gmail.com, andres@anarazel.de, petr@2ndquadrant.com, peter_e@gmx.net
Subject: Re: logical replication launcher crash on buildfarm
In-Reply-To: <e3c35f3a-426d-b1ca-7fe5-9241c2717013@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox