Horton
Menu

Thread

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

    Bruce Momjian <bruce@momjian.us> — 2012-11-30T04:12:23Z

    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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-11-30T04:23:59Z

    Bruce Momjian <bruce@momjian.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

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

    Bruce Momjian <bruce@momjian.us> — 2012-11-30T19:50:40Z

    On Thu, Nov 29, 2012 at 11:23:59PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

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

    Bruce Momjian <bruce@momjian.us> — 2012-11-30T21:45:18Z

    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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

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

    Andrew Dunstan <andrew@dunslane.net> — 2012-11-30T23:06:39Z

    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

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

    Bruce Momjian <bruce@momjian.us> — 2012-11-30T23:09:25Z

    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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

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

    Bruce Momjian <bruce@momjian.us> — 2012-12-04T04:24:53Z

    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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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