Where have the reviewers gone?

The PostgreSQL relational database management system is an important free software project, providing a solid and capable database system. As of this writing, the project's development roadmap states that the code has been in a feature freeze since the beginning of April, with all candidate patches up for review and merging into the source repository. That these patches will be fixed up, if necessary, and everything will come together for a planned 8.3.0 release in July.

PostgreSQL hacker Bruce Momjian recently sounded an alarm about this release:

It seems that the pile of candidate patches is not being reviewed in any sort of timely manner. So they remain candidates and the 8.3.0 release fails to take form. There are, it seems, a few directions the project could take in response to this problem:

• Continue with business as usual and ship a very late release. Certainly PostgreSQL would not be the first free software project to take that approach.

• Reopen development so that simpler patches which are not currently in the queue could be considered.

• Defer all patches which have not been reviewed and ship 8.3.0 with the code which is in the repository now. Some developers are in favor of this course, but others see it as unfair to the developers who have had patches in waiting since well before the 8.3 cycle began.

• Simply shove all the pending code into the repository and hope for the best. This one seems unlikely: the PostgreSQL hackers have a hard-won reputation for exceedingly solid code that they will not want to put at risk just to get a release out more quickly.

As of this writing, the project has not announced any decisions, but the developers have decided not to change the project July release date - yet. Stay tuned and we will eventually see what comes out.

This episode points out, once again, an important aspect of our process: often the limiting factor in a free software project is not the number of developers, but, instead, the number of reviewers who can look over the code those developers create. This shortage is especially acute in projects (like PostgreSQL) which insist on relatively rigorous review processes. But, in any project, the "many eyeballs" factor is important; if the eyeballs are not there, the free software process is not working as well as it should.

Reviewing patches can be unrewarding work. It requires great attention to detail and the energy to look for bugs in code without the corresponding gratification of having actually written that code. Poking holes in other peoples' work is not fun, especially when those people do not react well. Often it can seem like the same mistakes come around again and again, leading reviewers to wonder if anybody is actually listening to them. So reviewers can get a little grumpy at times. The people who can do the best reviews are usually also some of the project's best developers, and, often, they would rather be developing. So code piles up and the review does not happen.

In the long term, the community will need to find ways to encourage more code review if we are serious about the quality of the work we are doing. Better public acknowledgment of reviewers might be a good start; credit is, after all, one of the chief currencies in which we trade. Perhaps we need some better tools to support the review process; along these lines, the recently announced review board project is worthy of some attention. Some projects have considered requiring potential contributors to review some patches before their own contributions can be merged. The right way to encourage more code review will probably differ from one project to the next, but, one way or another, we can certainly find solutions to this problem.

---

to post comments

Where have the reviewers gone?

Posted May 24, 2007 3:47 UTC (Thu) by neilbrown (subscriber, #359) [Link] (5 responses)

Reviewing patches can be unrewarding work.

While I completely agree with this statement, I would like to assert that it can also be very rewarding work. I have had a couple of experiences recently where some review work was quite fun.

I think that a key element of enjoyment is the opportunity to meaningfully contribute. If the code is near-perfect, then there is not much room for contribution, and so not much pleasure. On the other hand, if the code is at total mess, then there is not much room for meaningful contribution either: "where do I start...."

But there is plenty of middle ground. Code that fits the coding standards of the project and has been tested for a couple of test cases, but probably still has a few warts is the sort of code that can be fun to review. The reviewer can learn something. They can experiment. And they can probably find a few bugs or room for simplification. While the author could probably find those bugs too if they looked a bit longer, letting the reviewer have a chance is just "Good sportsmanship".

I think the maxim "Release early, release often" is still good, but to please reviewers I would add "not too early, not too often". You don't want the reviewer to give up in disgust, or to get reviewer-fatigue.

I think it is important to have developer/reviewer teams (and yes, we need more of them), and it is important that they are both involved from early on. Getting code reviewed shouldn't be the last step. It should be integral to the development.

Where have the reviewers gone?

Posted May 24, 2007 8:52 UTC (Thu) by nix (subscriber, #2304) [Link] (4 responses)

People have commented in the past that if they turn out near-perfect code
it takes longer to get reviewed than if they turn out code with a few
intentional warts in it that the reviewer can pounce on :) (obviously if
the reviewer doesn't fix them, the submitter can, as they're intentionally
introduced!)

Where have the reviewers gone?

Posted May 24, 2007 9:33 UTC (Thu) by MathFox (guest, #6104) [Link] (3 responses)

It very much depends on the review-instructions. I have done code reviews on various (proprietary) projects and on one project we had an pretty simple and effective review guideline:

Would you accept the responsibility to maintain (fix bugs) in the code?

The goal wan not to get 100% bug free code, but to get clean and fixable code. (If we saw flaws, we pointed them out; most of them were then quickly fixed.) I can imagine that PostgreSQL places the bar a little higher and looks for data integrity and security issues too.

Anyway, it is reasonable to expect a peer review to take 5-10% of the coding time; when you need more, the code has become too complex and is ripe for a (re)design.

Where have the reviewers gone?

Posted May 24, 2007 13:56 UTC (Thu) by nix (subscriber, #2304) [Link] (2 responses)

Oh dear, that wouldn't work very well with me, or with anyone else who has a chronic inability to say no :/

Where have the reviewers gone?

Posted May 24, 2007 19:28 UTC (Thu) by smoogen (subscriber, #97) [Link] (1 responses)

Well when we dealt with it.. we stuck the person who said yes until we either realized that their input was useless or they really meant that they would support the code. The opposite also showed up. If a person constantly said no because they didnt want to do the work later of supporting it.. they found that their votes were less valued (or asked for).

Where have the reviewers gone?

Posted May 24, 2007 23:46 UTC (Thu) by giraffedata (guest, #1954) [Link]

I think that's a very vague standard. I myself would pass just about any code under that standard, because if I were to have responsibility for supporting it, I could just rewrite it all to meet my requirements. I have in fact accepted responsibility for supporting code (often for money) that I thought was poorly written and had various apparent bugs.

And on the other hand, I would hate to have someone review my own code under that standard, because a reviewer can interpret it as meaning if my code isn't written the way he would do it, I can't ship it. I.e. I must function as some stranger's lackey.

A clear definition of the meaning of a review is essential to eliminating the arguments, frustrations, and hurt feelings that turn people off of both sides of reviewing (don't want to review; don't want to submit code for review). And yet, I almost never see it. And, strangely, most people don't seem to notice it missing.

There is quite a variety of activity that can be a review.

One kind of a review is simply advice. You're pointing out to the coder things he may have missed. You've satisfied your obligation if you've taken a reasonable look at the code and communicated every issue that you think he may have overlooked. The code is the responsibility of the coder.

On the other end of the scale is acceptance of the code. The reviewer owns it and has full responsibility for it. He can reject it for any reason without even stating a reason -- correctness, readability, indentation, uselessness. He can also accept it without reading it.

My point is that if the coder and reviewer are working off different definitions, the review is likely to be unpleasant for both and I could see how people wouldn't volunteer to review code.

PostgreSQL 7.x good enough?

Posted May 24, 2007 15:09 UTC (Thu) by rwmj (guest, #5474) [Link] (3 responses)

I'm an avid user of PostgreSQL ... 7.4. It does everything I want, and my needs run to a few popular but not huge websites. I understand how it works, it's reliable to the point where it "just works" provided I schedule a vacuum once in a while.

I wonder if the problem is that it's just good enough, and these patches represent features which by and large people don't need?

Rich.

PostgreSQL 7.x good enough?

Posted May 24, 2007 17:45 UTC (Thu) by Dom2 (guest, #458) [Link]

I've found good speed ups by migrating to newer releases. Mind you, we have a moderately substantial amount of data. The other reason to upgrade is to get bug fixes. I'm not sure if 7.4.x is still supported…

PostgreSQL 7.x good enough?

Posted May 24, 2007 23:50 UTC (Thu) by giraffedata (guest, #1954) [Link]

Good point. If someone submits a patch and nobody reviews it, that's like a motion without a second. Maybe they should just ship the release without the unreviewed patches. And maybe patch submitters should line up a partner/sponsor before spending much time preparing a patch.

PostgreSQL 7.x good enough?

Posted May 25, 2007 23:00 UTC (Fri) by decibel (guest, #45434) [Link]

"I wonder if the problem is that it's just good enough, and these patches represent features which by and large people don't need?"

Yes, because all the PostgreSQL hackers are so bored that they just dream up useless features. ;)

Actually, if you look at what's in the patch queue, the issue is that these patches are extremely large and complex. Most of the easy to improve things have already been done, so making further improvements to the database becomes increasingly more difficult. But these improvements are very valuable. Probably the most important one is "HOT", which greatly reduces the need for vacuuming. There are lots of users that want that capability.

If 7.4 fits the bill for you, there's no huge reason to upgrade, though you should at least run the latest 7.4 version. You should also keep an eye on the announce list since 7.4 will likely be de-supported in the next few years.

Where have the reviewers gone? ... redux

Posted May 24, 2007 20:05 UTC (Thu) by pimlott (guest, #1535) [Link] (1 responses)

Forgive me, but I find it jarring that this article does not cite the previous one with almost exactly the same title. The PostgreSQL developers had this problem in the last release cycle, and LWN covered it then too.

Where have the reviewers gone? ... redux

Posted May 31, 2007 5:13 UTC (Thu) by slamb (guest, #1070) [Link]

Ahh...I had wondered if I'd become psychic or had jumped back in time or something.