Re: Commitfest problems

On 14/12/14 18:24, Peter Geoghegan wrote:

> On Sun, Dec 14, 2014 at 9:05 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> TBH, I'm not really on board with this line of argument. I don't find
>> broken-down patches to be particularly useful for review purposes. An
>> example I was just fooling with this week is the GROUPING SETS patch,
>> which was broken into three sections for no good reason at all. (The
>> fourth and fifth subpatches, being alternative solutions to one problem,
>> are in a different category of course.) Too often, decisions made in
>> one subpatch don't make any sense until you see the larger picture.
>
> It sounds like they didn't use the technique effectively, then. I
> think it will be useful to reviewers that I've broken out the
> mechanism through which the ON CONFLICT UPDATE patch accepts the
> EXCLUDED.* pseudo-alias into a separate commit (plus docs in a
> separate commit, as well as tests) - it's a non-trivial additional
> piece of code that it wouldn't be outrageous to omit in a release, and
> isn't particularly anticipated by earlier cumulative commits. Maybe
> people don't have a good enough sense of what sort of subdivision is
> appropriate yet. I think that better style will emerge over time.

For me this is a great example of how to get more developers involved in
patch review. Imagine that I'm an experienced developer with little
previous exposure to PostgreSQL, but with a really strong flex/bison
background.

If someone posts a patch to the list as a single grouping sets patch,
then I'm going to look at that and think "I have no way of understanding
this" and do nothing with it.

However if it were posted as part of patchset with a subject of "[PATCH]
gram.y: add EXCLUDED pseudo-alias to bison grammar" then this may pique
my interest enough to review the changes to the grammar rules, with the
barrier for entry reduced to understanding just the PostgreSQL-specific
parts.

What should happen over time is that developers like this would earn the
trust of the committers, so committers can spend more time reviewing the
remaining parts of the patchset. And of course the project has now
engaged a new developer into the community who otherwise may not have
not participated.

> Of course, if you still prefer a monolithic commit, it's pretty easy
> to produce one from a patch series. It's not easy to go in the other
> direction, though.

Agreed.

ATB,

Mark.