2010/7/31 Greg Smith <g...@2ndquadrant.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 > g...@2ndquadrant.com www.2ndQuadrant.us <http://www.2ndquadrant.us/> > >
