Обсуждение: 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.

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