Re: Patches that don't apply or don't compile: 2017-09-12

> On 12 Sep 2017, at 23:54, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On 09/12/2017 04:14 PM, Aleksander Alekseev wrote:
>> Hello, hackers!
>>
>> Thanks to the work of Thomas Munro now we have a CI for the patches on
>> the commitfest [1]. Naturally there is still room for improvement, but
>> in any case it's much, much better than nothing.
>>
>> After a short discussion [2] we agreed (or at least no one objected)
>> to determine the patches that don't apply / don't compile / don't
>> pass regression tests and have "Needs Review" status, change the
>> status of these patches to "Waiting on Author" and write a report
>> (this one) with a CC to the authors. As all we know, we are short on
>> reviewers and this action will save them a lot of time. Here [3] you
>> can find a script I've been using to find such patches.
>>
>> I rechecked the list manually and did my best to exclude the patches
>> that were updated recently or that depend on other patches. However
>> there still a chance that your patch got to the list by a mistake.
>> In this case please let me know.>
>
> With all due respect, it's hard not to see this as a disruption of the
> current CF. I agree automating the patch processing is a worthwhile
> goal, but we're not there yet and it seems somewhat premature.
>
> Let me explain why I think so:
>
> (1) You just changed the status of 10-15% open patches. I'd expect
> things like this to be consulted with the CF manager, yet I don't see
> any comments from Daniel. Considering he's been at the Oslo PUG meetup
> today, I doubt he was watching hackers very closely.

Correct, I’ve been travelling and running a meetup today so had missed this on
-hackers.

> (2) You gave everyone about 4 hours to object, ending 3PM UTC, which
> excludes about one whole hemisphere where it's either too early or too
> late for people to respond. I'd say waiting for >24 hours would be more
> appropriate.

Agreed.

> I object to changing the patch status merely based on the script output.
> It's a nice goal, but we need to do the legwork first, otherwise it'll
> be just annoying and disrupting.

I too fear that automating the state change will move patches away from “Needs
review” in too many cases unless there is manual inspection step. Colliding on
Oids in pg_proc comes to mind as a case where the patch won’t build, but the
reviewer can trivially fix that locally and keep reviewing.

> I suggest we inspect the reported patches manually, investigate whether
> the failures are legitimate or not, and eliminate as many false
> positives as possible. Once we are happy with the accuracy, we can
> enable it again.

This seems to summarize the sentiment in the other thread, this is absolutely a
step in the right direction, we just need to tweak it with human knowledge
before it can be made fully automatic to avoid false positives. The last thing
we want is for the community to consider CF status changes/updates to be crying
wolf, there are few enough reviewers as there is.

cheers ./daniel