Re: initdb.c::main() too large

In looking to add an fsync-only option to initdb, I found its main()
function to be 743 lines long, and very hard to understand.

The attached patch moves much of that code into separate functions,
which will make initdb.c easier to understand, and easier to add an
fsync-only option. The original initdb.c author, Andrew Dunstan, has
accepted the restructuring, in principle.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> In looking to add an fsync-only option to initdb, I found its main()
> function to be 743 lines long, and very hard to understand.

> The attached patch moves much of that code into separate functions,
> which will make initdb.c easier to understand, and easier to add an
> fsync-only option. The original initdb.c author, Andrew Dunstan, has
> accepted the restructuring, in principle.

No objection to breaking it into multiple functions --- but I do say
it's a lousy idea to put the long_options[] constant at the front of
the file, thousands of lines away from the switch construct that it
has to be in sync with. We don't do that in any other program AFAIR.
Keep that in the main() function, please.

regards, tom lane

On Thu, Nov 29, 2012 at 11:23:59PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > In looking to add an fsync-only option to initdb, I found its main()
> > function to be 743 lines long, and very hard to understand.
>
> > The attached patch moves much of that code into separate functions,
> > which will make initdb.c easier to understand, and easier to add an
> > fsync-only option. The original initdb.c author, Andrew Dunstan, has
> > accepted the restructuring, in principle.
>
> No objection to breaking it into multiple functions --- but I do say
> it's a lousy idea to put the long_options[] constant at the front of
> the file, thousands of lines away from the switch construct that it
> has to be in sync with. We don't do that in any other program AFAIR.
> Keep that in the main() function, please.

Good point. I had not noticed that. I fixed my initdb patch, and
adjusted a few other C files to be consistent.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Thu, Nov 29, 2012 at 11:12:23PM -0500, Bruce Momjian wrote:
> In looking to add an fsync-only option to initdb, I found its main()
> function to be 743 lines long, and very hard to understand.
>
> The attached patch moves much of that code into separate functions,
> which will make initdb.c easier to understand, and easier to add an
> fsync-only option. The original initdb.c author, Andrew Dunstan, has
> accepted the restructuring, in principle.

Applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 11/30/2012 04:45 PM, Bruce Momjian wrote:
> On Thu, Nov 29, 2012 at 11:12:23PM -0500, Bruce Momjian wrote:
>> In looking to add an fsync-only option to initdb, I found its main()
>> function to be 743 lines long, and very hard to understand.
>>
>> The attached patch moves much of that code into separate functions,
>> which will make initdb.c easier to understand, and easier to add an
>> fsync-only option. The original initdb.c author, Andrew Dunstan, has
>> accepted the restructuring, in principle.
> Applied.
>

Sorry I didn't have time to review this before it was applied.

A few minor nitpicks:

* process() is a fairly uninformative function name, not sure what I'd
call it, but something more descriptive.
* the setup_signals_and_umask() call and possibly the final message
section of process() would be better placed back in main() IMNSHO.
* the large statements for setting up the datadir and the xlogdir
should be factored out of process() into their own functions, I
think. That would make it much more readable.

cheers

andrew

On Fri, Nov 30, 2012 at 06:06:39PM -0500, Andrew Dunstan wrote:
>
> On 11/30/2012 04:45 PM, Bruce Momjian wrote:
> >On Thu, Nov 29, 2012 at 11:12:23PM -0500, Bruce Momjian wrote:
> >>In looking to add an fsync-only option to initdb, I found its main()
> >>function to be 743 lines long, and very hard to understand.
> >>
> >>The attached patch moves much of that code into separate functions,
> >>which will make initdb.c easier to understand, and easier to add an
> >>fsync-only option. The original initdb.c author, Andrew Dunstan, has
> >>accepted the restructuring, in principle.
> >Applied.
> >
>
> Sorry I didn't have time to review this before it was applied.
>
> A few minor nitpicks:
>
> * process() is a fairly uninformative function name, not sure what I'd
> call it, but something more descriptive.
> * the setup_signals_and_umask() call and possibly the final message
> section of process() would be better placed back in main() IMNSHO.
> * the large statements for setting up the datadir and the xlogdir
> should be factored out of process() into their own functions, I
> think. That would make it much more readable.

OK, I will make those changes. Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Nov 30, 2012 at 06:06:39PM -0500, Andrew Dunstan wrote:
>
> On 11/30/2012 04:45 PM, Bruce Momjian wrote:
> >On Thu, Nov 29, 2012 at 11:12:23PM -0500, Bruce Momjian wrote:
> >>In looking to add an fsync-only option to initdb, I found its main()
> >>function to be 743 lines long, and very hard to understand.
> >>
> >>The attached patch moves much of that code into separate functions,
> >>which will make initdb.c easier to understand, and easier to add an
> >>fsync-only option. The original initdb.c author, Andrew Dunstan, has
> >>accepted the restructuring, in principle.
> >Applied.
> >
>
> Sorry I didn't have time to review this before it was applied.
>
> A few minor nitpicks:
>
> * process() is a fairly uninformative function name, not sure what I'd
> call it, but something more descriptive.
> * the setup_signals_and_umask() call and possibly the final message
> section of process() would be better placed back in main() IMNSHO.
> * the large statements for setting up the datadir and the xlogdir
> should be factored out of process() into their own functions, I
> think. That would make it much more readable.

Done with the attached patch. I kept the signals in their own function,
but moved the umask() call out --- I was not happy mixing those either.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +