Re: Commitfest problems

On 15/12/14 19:08, Robert Haas wrote:

> On Sun, Dec 14, 2014 at 2:24 PM, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>> 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.
> 
> Meh.  Such a patch couldn't possibly compile or work without modifying
> other parts of the tree.  And I'm emphatically opposed to splitting a
> commit that would have taken the tree from one working state to
> another working state into a series of patches that would have taken
> it from a working state through various non-working states and
> eventually another working state.  Furthermore, if you really want to
> review that specific part of the patch, just look for what changed in
> gram.y, perhaps by applying the patch and doing git diff
> src/backend/parser/gram.y.  This is really not hard.

Okay I agree that the suggested subject above was a little misleading,
so let me try and clarify this further.

If I were aiming to deliver this as an individual patch as part of a
patchset, my target for this particular patch would be to alter both the
bison grammar *and* the minimal underlying code/structure changes into a
format that compiles but adds no new features.

So why is this useful? Because the parser in PostgreSQL is important and
people have sweated many hours to ensure its performance is the best it
can be. By having a checkpoint with just the basic parser changes then
it becomes really easy to bisect performance issues down to just this
one particular area, rather than the feature itself.

And as per my original message it is a much lower barrier of entry for a
potential reviewer who has previous bison experience (and is curious
about PostgreSQL) to review the basic rule changes than a complete new
feature.

> I certainly agree that there are cases where patch authors could and
> should put more work into decomposing large patches into bite-sized
> chunks.  But I don't think that's always possible, and I don't think
> that, for example, applying BRIN in N separate pieces would have been
> a step forward.  It's one feature, not many.

I completely agree with you here. Maybe this isn't exactly the same for
PostgreSQL but in general for a new QEMU feature I could expect a
patchset of around 12 patches to be posted. Of those 12 patches,
probably patches 1-9 are internal API changes, code refactoring and
preparation work, patch 10 is a larger patch containing the feature, and
patches 11-12 are for tidy-up and removing unused code sections.

Even with the best patch review process in the world, there will still
be patches that introduce bugs, and the bugs are pretty much guaranteed
to be caused by patches 1-9.

Imagine a subtle bug in an internal API change which exhibits itself not
in the feature added by the patchset itself but in another mostly
unrelated part of the system; maybe this could be caused by a
pass-by-value API being changed to a pass-by-reference API in patches
1-9 and this tickles a bug due to an unexpected lifecycle heap access
elsewhere causing random data to be returned. Being able to bisect this
issue down to a single specific patch suddenly becomes very useful indeed.


ATB,

Mark.