Re: merge command - GSoC progress

Boxuan Zhai wrote:
> I create a clean patch file (no debug clauses). See the attachment.

Some general coding feedback for you on this.

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.

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.

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.

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@2ndQuadrant.com   www.2ndQuadrant.us