On Thu, May 9, 2024 at 11:19 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Heikki Linnakangas <hlinn...@iki.fi> writes:
> > At first I was sure this was introduced by my refactorings in v17, but
> > in fact it's been like this forever. I agree that InitProcessing makes
> > much more sense. The ProcessingMode variable is initialized to
> > InitProcessing, so I think we can simply remove that line from
> > AuxiliaryProcessMainCommon(). There are existing
> > "SetProcessingMode(InitProcessing)" calls in other Main functions too
> > (AutoVacLauncherMain, BackgroundWorkerMain, etc.), and I think those can
> > also be removed.
>
> This only works if the postmaster can be trusted never to change the
> variable; else children could inherit some other value via fork().
> In that connection, it seems a bit scary that postmaster.c contains a
> couple of calls "SetProcessingMode(NormalProcessing)". It looks like
> they are in functions that should only be executed by child processes,
> but should we try to move them somewhere else?
After checking calls to "SetProcessingMode(NormalProcessing)" in the
postmaster.c, they are used in background worker specific functions
(BackgroundWorkerInitializeConnectionByOid and
BackgroundWorkerInitializeConnection). So I think it's a good idea to
move these functions to bgworker.c. Then, we can get rid of calling
"SetProcessingMode(NormalProcessing)" in postmaster.c.
I also noticed that there's an unnecessary call to
"BackgroundWorkerInitializeConnection" in worker_spi.c (The worker_spi
launcher has set the dboid correctly).
Best Regards,
Xing.
From 4dbe54ef746203740202e181f731b7e08bb6e4ea Mon Sep 17 00:00:00 2001
From: Xing Guo <higuox...@gmail.com>
Date: Fri, 10 May 2024 10:34:09 +0800
Subject: [PATCH v4 1/3] Move bgworker specific logic to bgworker.c.
---
src/backend/postmaster/bgworker.c | 83 +++++++++++++++++++++++++++++
src/backend/postmaster/postmaster.c | 83 -----------------------------
2 files changed, 83 insertions(+), 83 deletions(-)
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index cf64a4beb2..0ab64ae3ec 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -1240,6 +1240,89 @@ TerminateBackgroundWorker(BackgroundWorkerHandle *handle)
SendPostmasterSignal(PMSIGNAL_BACKGROUND_WORKER_CHANGE);
}
+/*
+ * Connect background worker to a database.
+ */
+void
+BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags)
+{
+ BackgroundWorker *worker = MyBgworkerEntry;
+ bits32 init_flags = 0; /* never honor session_preload_libraries */
+
+ /* ignore datallowconn? */
+ if (flags & BGWORKER_BYPASS_ALLOWCONN)
+ init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+ /* ignore rolcanlogin? */
+ if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+ init_flags |= INIT_PG_OVERRIDE_ROLE_LOGIN;
+
+ /* XXX is this the right errcode? */
+ if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
+ ereport(FATAL,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("database connection requirement not indicated during registration")));
+
+ InitPostgres(dbname, InvalidOid, /* database to connect to */
+ username, InvalidOid, /* role to connect as */
+ init_flags,
+ NULL); /* no out_dbname */
+
+ /* it had better not gotten out of "init" mode yet */
+ if (!IsInitProcessingMode())
+ ereport(ERROR,
+ (errmsg("invalid processing mode in background worker")));
+ SetProcessingMode(NormalProcessing);
+}
+
+/*
+ * Connect background worker to a database using OIDs.
+ */
+void
+BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
+{
+ BackgroundWorker *worker = MyBgworkerEntry;
+ bits32 init_flags = 0; /* never honor session_preload_libraries */
+
+ /* ignore datallowconn? */
+ if (flags & BGWORKER_BYPASS_ALLOWCONN)
+ init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
+ /* ignore rolcanlogin? */
+ if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
+ init_flags |= INIT_PG_OVERRIDE_ROLE_LOGIN;
+
+ /* XXX is this the right errcode? */
+ if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
+ ereport(FATAL,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("database connection requirement not indicated during registration")));
+
+ InitPostgres(NULL, dboid, /* database to connect to */
+ NULL, useroid, /* role to connect as */
+ init_flags,
+ NULL); /* no out_dbname */
+
+ /* it had better not gotten out of "init" mode yet */
+ if (!IsInitProcessingMode())
+ ereport(ERROR,
+ (errmsg("invalid processing mode in background worker")));
+ SetProcessingMode(NormalProcessing);
+}
+
+/*
+ * Block/unblock signals in a background worker
+ */
+void
+BackgroundWorkerBlockSignals(void)
+{
+ sigprocmask(SIG_SETMASK, &BlockSig, NULL);
+}
+
+void
+BackgroundWorkerUnblockSignals(void)
+{
+ sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
+}
+
/*
* Look up (and possibly load) a bgworker entry point function.
*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7f3170a8f0..472eb80fd1 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4148,89 +4148,6 @@ MaxLivePostmasterChildren(void)
max_wal_senders + max_worker_processes);
}
-/*
- * Connect background worker to a database.
- */
-void
-BackgroundWorkerInitializeConnection(const char *dbname, const char *username, uint32 flags)
-{
- BackgroundWorker *worker = MyBgworkerEntry;
- bits32 init_flags = 0; /* never honor session_preload_libraries */
-
- /* ignore datallowconn? */
- if (flags & BGWORKER_BYPASS_ALLOWCONN)
- init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
- /* ignore rolcanlogin? */
- if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
- init_flags |= INIT_PG_OVERRIDE_ROLE_LOGIN;
-
- /* XXX is this the right errcode? */
- if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
- ereport(FATAL,
- (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
- errmsg("database connection requirement not indicated during registration")));
-
- InitPostgres(dbname, InvalidOid, /* database to connect to */
- username, InvalidOid, /* role to connect as */
- init_flags,
- NULL); /* no out_dbname */
-
- /* it had better not gotten out of "init" mode yet */
- if (!IsInitProcessingMode())
- ereport(ERROR,
- (errmsg("invalid processing mode in background worker")));
- SetProcessingMode(NormalProcessing);
-}
-
-/*
- * Connect background worker to a database using OIDs.
- */
-void
-BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
-{
- BackgroundWorker *worker = MyBgworkerEntry;
- bits32 init_flags = 0; /* never honor session_preload_libraries */
-
- /* ignore datallowconn? */
- if (flags & BGWORKER_BYPASS_ALLOWCONN)
- init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
- /* ignore rolcanlogin? */
- if (flags & BGWORKER_BYPASS_ROLELOGINCHECK)
- init_flags |= INIT_PG_OVERRIDE_ROLE_LOGIN;
-
- /* XXX is this the right errcode? */
- if (!(worker->bgw_flags & BGWORKER_BACKEND_DATABASE_CONNECTION))
- ereport(FATAL,
- (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
- errmsg("database connection requirement not indicated during registration")));
-
- InitPostgres(NULL, dboid, /* database to connect to */
- NULL, useroid, /* role to connect as */
- init_flags,
- NULL); /* no out_dbname */
-
- /* it had better not gotten out of "init" mode yet */
- if (!IsInitProcessingMode())
- ereport(ERROR,
- (errmsg("invalid processing mode in background worker")));
- SetProcessingMode(NormalProcessing);
-}
-
-/*
- * Block/unblock signals in a background worker
- */
-void
-BackgroundWorkerBlockSignals(void)
-{
- sigprocmask(SIG_SETMASK, &BlockSig, NULL);
-}
-
-void
-BackgroundWorkerUnblockSignals(void)
-{
- sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
-}
-
/*
* Start a new bgworker.
* Starting time conditions must have been checked already.
--
2.45.0
From fa210ff72c4e4e55ee2d339070a142c134b3650c Mon Sep 17 00:00:00 2001
From: Xing Guo <higuox...@gmail.com>
Date: Fri, 10 May 2024 10:31:19 +0800
Subject: [PATCH v4 2/3] Remove redundant SetProcessingMode(InitProcessing)
calls.
After several refactoring iterations, auxiliary processes are no longer
initialized from the bootstrapper. Using the InitProcessing mode for
initializing auxiliary processes is more appropriate. Since the global
variable Mode is initialized to InitProcessing, we can remove redundant
calls of SetProcessingMode(InitProcessing).
---
src/backend/postmaster/autovacuum.c | 4 ++--
src/backend/postmaster/auxprocess.c | 3 ++-
src/backend/postmaster/bgworker.c | 2 +-
src/backend/replication/logical/slotsync.c | 2 +-
src/backend/tcop/postgres.c | 2 +-
5 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 9a925a10cd..928754b51c 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -380,7 +380,7 @@ AutoVacLauncherMain(char *startup_data, size_t startup_data_len)
if (PostAuthDelay)
pg_usleep(PostAuthDelay * 1000000L);
- SetProcessingMode(InitProcessing);
+ Assert(GetProcessingMode() == InitProcessing);
/*
* Set up signal handlers. We operate on databases much like a regular
@@ -1373,7 +1373,7 @@ AutoVacWorkerMain(char *startup_data, size_t startup_data_len)
MyBackendType = B_AUTOVAC_WORKER;
init_ps_display(NULL);
- SetProcessingMode(InitProcessing);
+ Assert(GetProcessingMode() == InitProcessing);
/*
* Set up signal handlers. We operate on databases much like a regular
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 78f4263eeb..b21eae7136 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -54,7 +54,8 @@ AuxiliaryProcessMainCommon(void)
init_ps_display(NULL);
- SetProcessingMode(BootstrapProcessing);
+ Assert(GetProcessingMode() == InitProcessing);
+
IgnoreSystemIndexes = true;
/*
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 0ab64ae3ec..92ec1c0e94 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -746,7 +746,7 @@ BackgroundWorkerMain(char *startup_data, size_t startup_data_len)
MyBackendType = B_BG_WORKER;
init_ps_display(worker->bgw_name);
- SetProcessingMode(InitProcessing);
+ Assert(GetProcessingMode() == InitProcessing);
/* Apply PostAuthDelay */
if (PostAuthDelay > 0)
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index f1f44d41ef..2653ce0342 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -1342,7 +1342,7 @@ ReplSlotSyncWorkerMain(char *startup_data, size_t startup_data_len)
init_ps_display(NULL);
- SetProcessingMode(InitProcessing);
+ Assert(GetProcessingMode() == InitProcessing);
/*
* Create a per-backend PGPROC struct in shared memory. We must do this
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2dff28afce..b9fee13612 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4161,7 +4161,7 @@ PostgresMain(const char *dbname, const char *username)
Assert(dbname != NULL);
Assert(username != NULL);
- SetProcessingMode(InitProcessing);
+ Assert(GetProcessingMode() == InitProcessing);
/*
* Set up signal handlers. (InitPostmasterChild or InitStandaloneProcess
--
2.45.0
From c445fbefa327edb7518da88c2783c17e6715a572 Mon Sep 17 00:00:00 2001
From: Xing Guo <higuox...@gmail.com>
Date: Fri, 10 May 2024 10:46:04 +0800
Subject: [PATCH v4 3/3] Minor improvement to connection initialization for
worker_spi.
---
src/test/modules/worker_spi/worker_spi.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 7e1042f4ab..23f4b165dc 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -167,11 +167,8 @@ worker_spi_main(Datum main_arg)
BackgroundWorkerUnblockSignals();
/* Connect to our database */
- if (OidIsValid(dboid))
- BackgroundWorkerInitializeConnectionByOid(dboid, roleoid, flags);
- else
- BackgroundWorkerInitializeConnection(worker_spi_database,
- worker_spi_role, flags);
+ Assert(OidIsValid(dboid));
+ BackgroundWorkerInitializeConnectionByOid(dboid, roleoid, flags);
/*
* Disable parallel query for workers started with BYPASS_ALLOWCONN or
--
2.45.0
