Thread: 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.
Attachment
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] 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