Re: Commitfest problems - Mailing list pgsql-hackers
From | Mark Cave-Ayland |
---|---|
Subject | Re: Commitfest problems |
Date | |
Msg-id | 548FFD0C.2090602@ilande.co.uk Whole thread Raw |
In response to | Re: Commitfest problems (Robert Haas <robertmhaas@gmail.com>) |
List | pgsql-hackers |
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.
pgsql-hackers by date: