return values of backend sub-main functions
There is a bit of confusion around the return values and return
protocols of the sub-main functions in the backend (PostgresMain etc.).
Some functions are declared to return int but never return. It would be
useful to make this consistent by either making them return void or
making some use of the return value.
PostgresMain() is declared to return int but never returns at all, so
why not make it return void? main() in turn reads the exit code and
passes it to exit(). Similarly, PostmasterMain() never returns, but is
declared to return int.
BackendRun() is declared to return int, and that return value is
actually obtained from the return value of PostgresMain() (see above),
and the return value of BackendRun() is passed to proc_exit(). That
appears to be elaborate nonsense, because the only way to get out of
PostgresMain() is through calling proc_exit in the first place.
PostgresMain() calls proc_exit(WalSenderMain()), but WalSenderMain ends
by calling WalSndLoop which in turn ends by calling proc_exit.
By contrast, AuxiliaryProcessMain() and all the sub-Main() functions
called from there are declared to return void.
I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
WalSenderMain(), and WalSndLoop() to return void as well.
Peter Eisentraut <peter_e@gmx.net> writes:
I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
WalSenderMain(), and WalSndLoop() to return void as well.
I agree this code is not very consistent or useful, but one question:
what should the callers do if one of these functions *does* return?
regards, tom lane
On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
WalSenderMain(), and WalSndLoop() to return void as well.I agree this code is not very consistent or useful, but one question:
what should the callers do if one of these functions *does* return?
I was thinking of a two-pronged approach: First, add
__attribute__((noreturn)) to the functions. This will cause a suitable
compiler to verify on a source-code level that nothing actually returns
from the function. And second, at the call site, put an abort(); /*
not reached */. Together, this will make the code cleaner and more
consistent, and will also help the compiler out a bit about the control
flow.
On ons, 2012-01-18 at 21:21 +0200, Peter Eisentraut wrote:
On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
WalSenderMain(), and WalSndLoop() to return void as well.I agree this code is not very consistent or useful, but one question:
what should the callers do if one of these functions *does* return?I was thinking of a two-pronged approach: First, add
__attribute__((noreturn)) to the functions. This will cause a suitable
compiler to verify on a source-code level that nothing actually returns
from the function. And second, at the call site, put an abort(); /*
not reached */. Together, this will make the code cleaner and more
consistent, and will also help the compiler out a bit about the control
flow.
Patch for 9.3 attached.
Attachments:
pg-noreturn-backend.patchtext/x-patch; charset=UTF-8; name=pg-noreturn-backend.patchDownload
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index c7d48e9..33c5a0a 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -173,7 +173,7 @@
#ifdef EXEC_BACKEND
if (argc > 1 && strncmp(argv[1], "--fork", 6) == 0)
- exit(SubPostmasterMain(argc, argv));
+ SubPostmasterMain(argc, argv); /* does not return */
#endif
#ifdef WIN32
@@ -189,14 +189,13 @@
if (argc > 1 && strcmp(argv[1], "--boot") == 0)
AuxiliaryProcessMain(argc, argv); /* does not return */
-
- if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
- exit(GucInfoMain());
-
- if (argc > 1 && strcmp(argv[1], "--single") == 0)
- exit(PostgresMain(argc, argv, get_current_username(progname)));
-
- exit(PostmasterMain(argc, argv));
+ else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
+ GucInfoMain(); /* does not return */
+ else if (argc > 1 && strcmp(argv[1], "--single") == 0)
+ PostgresMain(argc, argv, get_current_username(progname)); /* does not return */
+ else
+ PostmasterMain(argc, argv); /* does not return */
+ abort(); /* should not get here */
}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index eeea933..0f8af3a 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -343,8 +343,8 @@ static void LogChildExit(int lev, const char *procname,
int pid, int exitstatus);
static void PostmasterStateMachine(void);
static void BackendInitialize(Port *port);
-static int BackendRun(Port *port);
-static void ExitPostmaster(int status);
+static void BackendRun(Port *port) __attribute__((noreturn));
+static void ExitPostmaster(int status) __attribute__((noreturn));
static int ServerLoop(void);
static int BackendStartup(Port *port);
static int ProcessStartupPacket(Port *port, bool SSLdone);
@@ -491,7 +491,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
/*
* Postmaster main entry point
*/
-int
+void
PostmasterMain(int argc, char *argv[])
{
int opt;
@@ -1125,7 +1125,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
*/
ExitPostmaster(status != STATUS_OK);
- return 0; /* not reached */
+ abort(); /* not reached */
}
@@ -3295,7 +3295,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
BackendInitialize(port);
/* And run the backend */
- proc_exit(BackendRun(port));
+ BackendRun(port);
}
#endif /* EXEC_BACKEND */
@@ -3539,7 +3539,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
* Shouldn't return at all.
* If PostgresMain() fails, return status.
*/
-static int
+static void
BackendRun(Port *port)
{
char **av;
@@ -3610,7 +3610,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
*/
MemoryContextSwitchTo(TopMemoryContext);
- return (PostgresMain(ac, av, port->user_name));
+ PostgresMain(ac, av, port->user_name);
}
@@ -3960,7 +3960,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
* have been inherited by fork() on Unix. Remaining arguments go to the
* subprocess FooMain() routine.
*/
-int
+void
SubPostmasterMain(int argc, char *argv[])
{
Port port;
@@ -4195,7 +4195,7 @@ static bool save_backend_variables(BackendParameters *param, Port *port,
proc_exit(0);
}
- return 1; /* shouldn't get here */
+ abort(); /* shouldn't get here */
}
#endif /* EXEC_BACKEND */
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 45a3b2e..a9a8689 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -121,7 +121,7 @@
/* Prototypes for private functions */
static bool HandleReplicationCommand(const char *cmd_string);
-static int WalSndLoop(void);
+static void WalSndLoop(void) __attribute__((noreturn));
static void InitWalSnd(void);
static void WalSndHandshake(void);
static void WalSndKill(int code, Datum arg);
@@ -136,7 +136,7 @@
/* Main entry point for walsender process */
-int
+void
WalSenderMain(void)
{
MemoryContext walsnd_context;
@@ -193,7 +193,7 @@
SyncRepInitConfig();
/* Main loop of walsender */
- return WalSndLoop();
+ WalSndLoop();
}
/*
@@ -708,7 +708,7 @@
}
/* Main loop of walsender process */
-static int
+static void
WalSndLoop(void)
{
char *output_message;
@@ -884,7 +884,7 @@
whereToSendOutput = DestNone;
proc_exit(0);
- return 1; /* keep the compiler quiet */
+ abort(); /* keep the compiler quiet */
}
/* Initialize a per-walsender data structure for this walsender process */
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 51b6df5..9a5438f 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3507,7 +3507,7 @@
* for the session.
* ----------------------------------------------------------------
*/
-int
+void
PostgresMain(int argc, char *argv[], const char *username)
{
const char *dbname;
@@ -3721,7 +3721,10 @@
/* If this is a WAL sender process, we're done with initialization. */
if (am_walsender)
- proc_exit(WalSenderMain());
+ {
+ WalSenderMain(); /* does not return */
+ abort();
+ }
/*
* process any libraries that should be preloaded at backend start (this
@@ -4199,7 +4202,7 @@
/* can't get here because the above loop never exits */
Assert(false);
- return 1; /* keep compiler quiet */
+ abort(); /* keep compiler quiet */
}
diff --git a/src/backend/utils/misc/help_config.c b/src/backend/utils/misc/help_config.c
index 5f772f9..386ec98 100644
--- a/src/backend/utils/misc/help_config.c
+++ b/src/backend/utils/misc/help_config.c
@@ -43,7 +43,7 @@
static bool displayStruct(mixedStruct *structToDisplay);
-int
+void
GucInfoMain(void)
{
struct config_generic **guc_vars;
@@ -64,7 +64,7 @@
printMixedStruct(var);
}
- return 0;
+ exit(0);
}
diff --git a/src/include/bootstrap/bootstrap.h b/src/include/bootstrap/bootstrap.h
index e966a73..b31bca9 100644
--- a/src/include/bootstrap/bootstrap.h
+++ b/src/include/bootstrap/bootstrap.h
@@ -40,7 +40,7 @@ extern Form_pg_attribute attrtypes[MAXATTR];
extern int numattr;
-extern void AuxiliaryProcessMain(int argc, char *argv[]);
+extern void AuxiliaryProcessMain(int argc, char *argv[]) __attribute__((noreturn));
extern void err_out(void);
diff --git a/src/include/postmaster/bgwriter.h b/src/include/postmaster/bgwriter.h
index f46d4ad..996065c 100644
--- a/src/include/postmaster/bgwriter.h
+++ b/src/include/postmaster/bgwriter.h
@@ -25,8 +25,8 @@ extern int CheckPointTimeout;
extern int CheckPointWarning;
extern double CheckPointCompletionTarget;
-extern void BackgroundWriterMain(void);
-extern void CheckpointerMain(void);
+extern void BackgroundWriterMain(void) __attribute__((noreturn));
+extern void CheckpointerMain(void) __attribute__((noreturn));
extern void RequestCheckpoint(int flags);
extern void CheckpointWriteDelay(int flags, double progress);
diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h
index 683ce3c..72c1423 100644
--- a/src/include/postmaster/postmaster.h
+++ b/src/include/postmaster/postmaster.h
@@ -46,7 +46,7 @@ extern int postmaster_alive_fds[2];
extern const char *progname;
-extern int PostmasterMain(int argc, char *argv[]);
+extern void PostmasterMain(int argc, char *argv[]) __attribute__((noreturn));
extern void ClosePostmasterPorts(bool am_syslogger);
extern int MaxLivePostmasterChildren(void);
diff --git a/src/include/postmaster/startup.h b/src/include/postmaster/startup.h
index 3ec6950..54400b5 100644
--- a/src/include/postmaster/startup.h
+++ b/src/include/postmaster/startup.h
@@ -13,7 +13,7 @@
#define _STARTUP_H
extern void HandleStartupProcInterrupts(void);
-extern void StartupProcessMain(void);
+extern void StartupProcessMain(void) __attribute__((noreturn));
extern void PreRestoreCommand(void);
extern void PostRestoreCommand(void);
extern bool IsPromoteTriggered(void);
diff --git a/src/include/postmaster/walwriter.h b/src/include/postmaster/walwriter.h
index 41c539a..922142a 100644
--- a/src/include/postmaster/walwriter.h
+++ b/src/include/postmaster/walwriter.h
@@ -15,6 +15,6 @@
/* GUC options */
extern int WalWriterDelay;
-extern void WalWriterMain(void);
+extern void WalWriterMain(void) __attribute__((noreturn));
#endif /* _WALWRITER_H */
diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index d21ec94..31449d2 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -109,7 +109,7 @@ typedef void (*walrcv_disconnect_type) (void);
extern PGDLLIMPORT walrcv_disconnect_type walrcv_disconnect;
/* prototypes for functions in walreceiver.c */
-extern void WalReceiverMain(void);
+extern void WalReceiverMain(void) __attribute__((noreturn));
/* prototypes for functions in walreceiverfuncs.c */
extern Size WalRcvShmemSize(void);
diff --git a/src/include/replication/walsender.h b/src/include/replication/walsender.h
index 128d2db..38b6e8e 100644
--- a/src/include/replication/walsender.h
+++ b/src/include/replication/walsender.h
@@ -26,7 +26,7 @@ extern volatile sig_atomic_t walsender_ready_to_stop;
extern int max_wal_senders;
extern int replication_timeout;
-extern int WalSenderMain(void);
+extern void WalSenderMain(void) __attribute__((noreturn));
extern void WalSndSignals(void);
extern Size WalSndShmemSize(void);
extern void WalSndShmemInit(void);
diff --git a/src/include/storage/ipc.h b/src/include/storage/ipc.h
index 16025c3..3bc2e58 100644
--- a/src/include/storage/ipc.h
+++ b/src/include/storage/ipc.h
@@ -64,7 +64,7 @@ typedef void (*shmem_startup_hook_type) (void);
/* ipc.c */
extern bool proc_exit_inprogress;
-extern void proc_exit(int code);
+extern void proc_exit(int code) __attribute__((noreturn));
extern void shmem_exit(int code);
extern void on_proc_exit(pg_on_exit_callback function, Datum arg);
extern void on_shmem_exit(pg_on_exit_callback function, Datum arg);
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index 964dd19..ccd2b89 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -61,7 +61,7 @@ extern bool check_max_stack_depth(int *newval, void **extra, GucSource source);
extern void assign_max_stack_depth(int newval, void *extra);
extern void die(SIGNAL_ARGS);
-extern void quickdie(SIGNAL_ARGS);
+extern void quickdie(SIGNAL_ARGS) __attribute__((noreturn));
extern void StatementCancelHandler(SIGNAL_ARGS);
extern void FloatExceptionHandler(SIGNAL_ARGS);
extern void RecoveryConflictInterrupt(ProcSignalReason reason); /* called from SIGUSR1
@@ -70,7 +70,7 @@ extern void prepare_for_client_read(void);
extern void client_read_ended(void);
extern const char *process_postgres_switches(int argc, char *argv[],
GucContext ctx);
-extern int PostgresMain(int argc, char *argv[], const char *username);
+extern void PostgresMain(int argc, char *argv[], const char *username) __attribute__((noreturn));
extern long get_stack_depth_rlimit(void);
extern void ResetUsage(void);
extern void ShowUsage(const char *title);
diff --git a/src/include/utils/help_config.h b/src/include/utils/help_config.h
index b569a4e..62d7e49 100644
--- a/src/include/utils/help_config.h
+++ b/src/include/utils/help_config.h
@@ -12,6 +12,6 @@
#ifndef HELP_CONFIG_H
#define HELP_CONFIG_H 1
-extern int GucInfoMain(void);
+extern void GucInfoMain(void) __attribute__((noreturn));
#endif
On Tue, Jun 19, 2012 at 7:31 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
On ons, 2012-01-18 at 21:21 +0200, Peter Eisentraut wrote:
On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
WalSenderMain(), and WalSndLoop() to return void as well.I agree this code is not very consistent or useful, but one question:
what should the callers do if one of these functions *does* return?I was thinking of a two-pronged approach: First, add
__attribute__((noreturn)) to the functions. This will cause a suitable
compiler to verify on a source-code level that nothing actually returns
from the function. And second, at the call site, put an abort(); /*
not reached */. Together, this will make the code cleaner and more
consistent, and will also help the compiler out a bit about the control
flow.Patch for 9.3 attached.
Seems reasonable on a quick read-through.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Jun 19, 2012 at 4:31 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
On ons, 2012-01-18 at 21:21 +0200, Peter Eisentraut wrote:
On lör, 2012-01-07 at 16:41 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
I suggest that we change PostgresMain(), PostmasterMain(), BackendRun(),
WalSenderMain(), and WalSndLoop() to return void as well.I agree this code is not very consistent or useful, but one question:
what should the callers do if one of these functions *does* return?I was thinking of a two-pronged approach: First, add
__attribute__((noreturn)) to the functions. This will cause a suitable
compiler to verify on a source-code level that nothing actually returns
from the function. And second, at the call site, put an abort(); /*
not reached */. Together, this will make the code cleaner and more
consistent, and will also help the compiler out a bit about the control
flow.Patch for 9.3 attached.
+1. Should this call around line 4114 of postmaster.c not bother with
proc_exit() anymore:
/* And run the backend */
proc_exit(BackendRun(&port));
I was hoping that some of the clang static analyzer complaints would
go away with these changes, though it looks like only one[1]this one goes away with the patch: http://kupershmidt.org/pg/scan-build-2012-06-19-master/report-E2cUqJ.html#EndPath did. I
would be interested to see the similar elog/ereport patch you
mentioned previously, perhaps that will eliminate a bunch of warnings.
Josh
[1]: this one goes away with the patch: http://kupershmidt.org/pg/scan-build-2012-06-19-master/report-E2cUqJ.html#EndPath
http://kupershmidt.org/pg/scan-build-2012-06-19-master/report-E2cUqJ.html#EndPath