Re: merge command - GSoC progress

2010/7/31 Greg Smith <greg(at)2ndquadrant(dot)com>

> Boxuan Zhai wrote:
>
>> I create a clean patch file (no debug clauses). See the attachment.
>>
>
> Some general coding feedback for you on this.
>
> Thanks for your consideration!

> It's not obvious to people who might want to try this out what exactly they
> are supposed to do. Particularly for complicated patches like this, where
> only a subset of the final feature might actually be implemented, some sort
> of reviewer guide suggesting what should and shouldn't work would be
> extremely helpful. I recall there was some sort of patch design guide in an
> earlier version of this patch; it doesn't seem to be there anymore. Don't
> remember if that had any examples in it.
>
> I am now working on fixing a bug which makes the system unable to initdb. I
will update my page in postgres Wiki with a detailed instruction of my
implementation and testing examples soon, with my next patch file.

> Ultimately, the examples of working behavior for this patch will need to be
> put into code. The way that happens is by adding working examples into the
> regression tests for the database. If you had those done for this patch, I
> wouldn't have to ask for code examples; I could just look at the source to
> the regression output and see how to use it against the standard database
> the regression samples create, and then execute against. This at least lets
> you avoid having to generate some test data, because there will already be
> some in the regression database for you to use. There is an intro this
> topic at http://wiki.postgresql.org/wiki/Regression_test_authoring Another common way to generate test data is to run pgbench which creates 4
> tables and populates them with data.
>
> I will try to add my testing examples to the gregression folder.

> As far as the patch itself goes, you have some work left on cleaning that
> up still you'll need to do eventually. What I would suggest is actually
> reading the patch itself; don't just generate it and send it, read through
> the whole thing like someone new to it would do. One way you can improve
> what you've done already is to find places where you have introduced changes
> to the code structure just through editing. Here's an example of what I'm
> talking about, from line 499 of your patch:
>
> - break;
> + break;
> This happened because you added two invisible tabs to the end of this line.
> This makes the patch larger for no good reason and tends to infuriate
> people who work on the code. There's quite a bit of extra lines added in
> here too that should go. You should consider reducing the ultimate size of
> the patch in terms of lines a worthwhile use of your time, even if it
> doesn't change how things work. There's lots of examples in this one where
> you put two or three lines between two statements when a single one would
> match the look of the code in that file. A round of reading the diff
> looking for that sort of problem would be useful.
>
> Another thing you should do is limit how long each line is when practical.
> You have lots of seriously wide comment lines here right now in particular.
> While there are some longer lines in the PostgreSQL code right now, that's
> code, not comments. And when you have a long line and a long comment, don't
> tack the comment onto the end. Put it on the line above instead. Also,
> don't use "//" in the middle of comments the way you've done in a few places
> here.
>
> Sorry for these mistakes, again. I promise that the same thing will not
happen in my next patch.

> Getting some examples sorted out and starting on regression tests is more
> important than the coding style parts I think, just wanted to pass those
> along while I noticed them reading the patch, so you could start looking out
> for them more as you continue to work on it.
>
> --
> Greg Smith 2ndQuadrant US Baltimore, MD
> PostgreSQL Training, Services and Support
> greg(at)2ndQuadrant(dot)com www.2ndQuadrant.us <http://www.2ndquadrant.us/>
>
>