Re: Commitfest problems

I have heard repeated concerns about the commitfest process in the past
few months. The fact we have been in a continual commitfest since
August also is concerning.

I think we are reaching the limits on how much we can do with our
existing commitfest structure, and it might be time to consider changes.
While the commitfest process hasn't changed much and was very successful
in the first few years, a few things have changed externally:

1 more new developers involved in contributing small patches
2 more full-time developers creating big patches
3 increased time demands on experienced Postgres developers

These changes are all driven by increased Postgres popularity. In an
ideal world, as 1 and 2 increase, the time experienced developers have
to work on commitfest items would also increase, but the opposite has
happened. Many of our most experienced developers (3) hold responsible
positions in their organizations which demand their time.

You would think that more full-time developers (2) would help with the
commitfest, but often their paid work is in a specific subsystem of
Postgres, preventing them from effectively helping with other complex
patches.

We have always known that we can create novice developers faster than
experienced ones, but it seems this difference is accelerating.

A good example is the UPSERT patch, which, while receiving extensive
feedback on the user interface and syntax, has not had a full review,
though it has been posted for several months.

It seems like a good time to consider options for restructuring our
process.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 12/10/2014 10:35 PM, Bruce Momjian wrote:
> I have heard repeated concerns about the commitfest process in the past
> few months. The fact we have been in a continual commitfest since
> August also is concerning.
>
> I think we are reaching the limits on how much we can do with our
> existing commitfest structure, and it might be time to consider changes.
> While the commitfest process hasn't changed much and was very successful
> in the first few years, a few things have changed externally:
>
> 1 more new developers involved in contributing small patches
> 2 more full-time developers creating big patches
> 3 increased time demands on experienced Postgres developers

I will add:

4. commitfest managers have burned out and refuse to do it again

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Dec 10, 2014 at 11:00:21PM -0800, Josh Berkus wrote:
> On 12/10/2014 10:35 PM, Bruce Momjian wrote:
> > I think we are reaching the limits on how much we can do with our
> > existing commitfest structure, and it might be time to consider changes.
> > While the commitfest process hasn't changed much and was very successful
> > in the first few years, a few things have changed externally:
> >
> > 1 more new developers involved in contributing small patches
> > 2 more full-time developers creating big patches
> > 3 increased time demands on experienced Postgres developers
>
> I will add:
>
> 4. commitfest managers have burned out and refuse to do it again

Agreed. The "fun", if it was ever there, has left the commitfest
process.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 12/11/14 1:35 AM, Bruce Momjian wrote:
> While the commitfest process hasn't changed much and was very successful
> in the first few years, a few things have changed externally:
>
> 1 more new developers involved in contributing small patches
> 2 more full-time developers creating big patches
> 3 increased time demands on experienced Postgres developers

The number of patches registered in the commit fests hasn't actually
changed over the years. It has always fluctuated between 50 and 100,
depending on the point of the release cycle. So I don't think (1) is
necessarily the problem.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 12/11/14 1:35 AM, Bruce Momjian wrote:
>> While the commitfest process hasn't changed much and was very successful
>> in the first few years, a few things have changed externally:
>>
>> 1 more new developers involved in contributing small patches
>> 2 more full-time developers creating big patches
>> 3 increased time demands on experienced Postgres developers

> The number of patches registered in the commit fests hasn't actually
> changed over the years. It has always fluctuated between 50 and 100,
> depending on the point of the release cycle. So I don't think (1) is
> necessarily the problem.

Interesting point, but of course not all patches are equal; perhaps
the problem is that we're seeing fewer of (1) and more of (2) in the
commitfest queue. Is there any easy way to quantify the size/complexity
of the patches in past fests?

regards, tom lane

On 12/11/2014 09:02 AM, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> On 12/11/14 1:35 AM, Bruce Momjian wrote:
>>> While the commitfest process hasn't changed much and was very successful
>>> in the first few years, a few things have changed externally:
>>>
>>> 1 more new developers involved in contributing small patches
>>> 2 more full-time developers creating big patches
>>> 3 increased time demands on experienced Postgres developers
>
>> The number of patches registered in the commit fests hasn't actually
>> changed over the years. It has always fluctuated between 50 and 100,
>> depending on the point of the release cycle. So I don't think (1) is
>> necessarily the problem.

I don't think that's accurate. The number of patches per CF *has* edged
upwards by 10-30% per CF over the years:

http://www.databasesoup.com/2013/08/94-commitfest-1-wrap-up.html

(I haven't done the rest of 9.4 yet or 9.5)

No, it's not a jump up by 2X, but it is an upwards trend. And I think
that Tom has it right that the additional patches we're seeing are
additional large, complex patches.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Dec 11, 2014 at 1:26 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> No, it's not a jump up by 2X, but it is an upwards trend. And I think
> that Tom has it right that the additional patches we're seeing are
> additional large, complex patches.

I feel like there's increasingly not that much easy stuff left to do.
Many of the problems we haven't solved yet are unsolved because they
are really hard, or because not that important, or both. For example,
if you look at the current CommitFest, there are things like:

Add ssl_protocols configuration option
alter user/role CURRENT_USER
Fix local_preload_libraries to work as expected.
Refactoring code for synchronous node detection
Refactor of functions related to quoting from builtins.h to utils/quote.h

Those things are all, obviously, important to somebody, or there
wouldn't be patches for them in need of review. But in the broader
scheme of things, they are very minor issues. And then you have
things like:

deparse DDL in event triggers
Compression of Full Page Writes
WIP: dynahash replacement for buffer table
Foreign table inheritance
Grouping Sets
INSERT ... ON CONFLICT {UPDATE | IGNORE}

Pretty much everything on that list is something we've been wrestling
with as a project for at least half a decade. I remember people
complaining about DDL triggers when I first got involved in the
PostgreSQL community, and the initial patch for foreign tables had
support for inheritance and constraints which I ripped out before
committing as too half-baked. Pavel had a GROUPING SETS patch by
2009. Brainstorming solutions to the full_page_writes problem has
been a perennial PGCon after-hours activity for as long as I've been
going. So it's natural that, to the extent these patches are making
progress, they're doing so slowly. It's hard stuff to get right.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Dec 11, 2014 at 11:49:43AM -0500, Peter Eisentraut wrote:
> On 12/11/14 1:35 AM, Bruce Momjian wrote:
> > While the commitfest process hasn't changed much and was very successful
> > in the first few years, a few things have changed externally:
> >
> > 1 more new developers involved in contributing small patches
> > 2 more full-time developers creating big patches
> > 3 increased time demands on experienced Postgres developers
>
> The number of patches registered in the commit fests hasn't actually
> changed over the years. It has always fluctuated between 50 and 100,
> depending on the point of the release cycle. So I don't think (1) is
> necessarily the problem.

Yes, I was hoping someone would produce some numbers --- thanks. I
think the big question is whether the level of complexity of the patches
has increased, or whether it is just the amount of experienced developer
time (3) that has decreased, or something else. Or maybe things have
not materially changed at all over the years.

I am following this thought process:

1. Do we have a problem?
2. What is the cause of the problem?
3. How do we fix the problem?

I think to get a useful outcome we have to process things in that order.
So, is #1 true, and if so, what is the answer to #2?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 12/11/14 1:56 PM, Robert Haas wrote:
> I feel like there's increasingly not that much easy stuff left to do.
> Many of the problems we haven't solved yet are unsolved because they
> are really hard, or because not that important, or both.

You know, I was telling people the very same thing in 2004.

Yet here we are.

Maybe we just need to relax and accept that we're awesome.

On 12/11/14 1:35 AM, Bruce Momjian wrote:
> I have heard repeated concerns about the commitfest process in the past
> few months. The fact we have been in a continual commitfest since
> August also is concerning.

I realized the other day, I'm embracing the idea of a continual commitfest.

I'm still working on patches from the last commitfest. Why not? They
didn't expire. Sure, it would have been nicer to get them done sooner,
but what are you gonna do? The fact that Nov 15 < now < Dec 15 isn't
going to change the fact that I have a few hours to spare right now and
the patches are still relevant.

As far as I'm concerned, we might as well just have one commitfest per
major release. Call it a patch list. Make the list sortable by created
date and last-updated date, and let the system police itself. At least
that's honest.

On Thu, Dec 11, 2014 at 03:47:29PM -0500, Peter Eisentraut wrote:
> On 12/11/14 1:35 AM, Bruce Momjian wrote:
> > I have heard repeated concerns about the commitfest process in the past
> > few months. The fact we have been in a continual commitfest since
> > August also is concerning.
>
> I realized the other day, I'm embracing the idea of a continual commitfest.
>
> I'm still working on patches from the last commitfest. Why not? They
> didn't expire. Sure, it would have been nicer to get them done sooner,
> but what are you gonna do? The fact that Nov 15 < now < Dec 15 isn't
> going to change the fact that I have a few hours to spare right now and
> the patches are still relevant.
>
> As far as I'm concerned, we might as well just have one commitfest per
> major release. Call it a patch list. Make the list sortable by created
> date and last-updated date, and let the system police itself. At least
> that's honest.

Wow, that's radical, and interesting.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/11/2014 01:07 PM, Bruce Momjian wrote:
> On Thu, Dec 11, 2014 at 03:47:29PM -0500, Peter Eisentraut wrote:
>> On 12/11/14 1:35 AM, Bruce Momjian wrote:
>>> I have heard repeated concerns about the commitfest process in
>>> the past few months. The fact we have been in a continual
>>> commitfest since August also is concerning.
>>
>> I realized the other day, I'm embracing the idea of a continual
>> commitfest.
>>
>> I'm still working on patches from the last commitfest. Why not?
>> They didn't expire. Sure, it would have been nicer to get them
>> done sooner, but what are you gonna do? The fact that Nov 15 <
>> now < Dec 15 isn't going to change the fact that I have a few
>> hours to spare right now and the patches are still relevant.
>>
>> As far as I'm concerned, we might as well just have one
>> commitfest per major release. Call it a patch list. Make the
>> list sortable by created date and last-updated date, and let the
>> system police itself. At least that's honest.
>
> Wow, that's radical, and interesting.

Actually, to me it sounds a lot like what we did 10 years ago before
the commitfests except with a way to track the patches (other than the
mail archives) and more people participating in patch reviews.

Joe

- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJUigi9AAoJEDfy90M199hlgTEP/3wZov8w32JN8Olp/KV8OCsC
ZwTluKRxgfAMf3bZmimxrnD5SYvgj6RhGwBfPvnNXPub+ODfCqTkMJ9dx41Xv/H9
wm0sFctfZTJb3TLbdZS4slg32LMbpCXIwL4QmNAXmkbKqqTVkTx+G2RvjtdkIpSQ
ZApg4S7LvDazACZjTSl+bUNfzvu9Ygl6Nu4otgXaMbM1ntfKtgF3irpf0+K8Wwi9
7zQ1eU+bSnH9+/kb416WufLutIP182OalbB3BI1KbLURTL98FElUmPYD7xV39OwZ
tE6bFNwUDYdrZCR6B+vkbf/z+9xa8C2vqU9d85MyKNjV1sB1MXX5O+yMRT7eQd8q
JOX1IhKOMMS79c1LqB2vTQlv8lU6VD6gvxFGO4X2OD5cGaLyl4L90hSYmZDordQe
uhwtCo0xoBFExaNb1NF9bzH8u0bJJpxDYZJPPhMNcWMY7EZ3+McXU7MxinDwLh1D
wNynzrZs0Oq8jyn0XwGSF54urkC5clmpEwnhlzeiu2oHosgKLOOw688r38O77MOL
EtNDcVSq2x4B6ocCjGEN7Xcbjm8kbiuQoQJBXUm2C/lvqbiNyzYJ19oH01tHcVTC
oMv3MrhlJ4p83t40bZGqoDUpA5/hme255OgqRdDDH7WFGQXwA6CQEsROFshcIcyR
f7Zx9iww4SGACOj+j0OS
=hSWH
-----END PGP SIGNATURE-----

On Thu, Dec 11, 2014 at 01:12:30PM -0800, Joe Conway wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 12/11/2014 01:07 PM, Bruce Momjian wrote:
> > On Thu, Dec 11, 2014 at 03:47:29PM -0500, Peter Eisentraut wrote:
> >> On 12/11/14 1:35 AM, Bruce Momjian wrote:
> >>> I have heard repeated concerns about the commitfest process in
> >>> the past few months. The fact we have been in a continual
> >>> commitfest since August also is concerning.
> >>
> >> I realized the other day, I'm embracing the idea of a continual
> >> commitfest.
> >>
> >> I'm still working on patches from the last commitfest. Why not?
> >> They didn't expire. Sure, it would have been nicer to get them
> >> done sooner, but what are you gonna do? The fact that Nov 15 <
> >> now < Dec 15 isn't going to change the fact that I have a few
> >> hours to spare right now and the patches are still relevant.
> >>
> >> As far as I'm concerned, we might as well just have one
> >> commitfest per major release. Call it a patch list. Make the
> >> list sortable by created date and last-updated date, and let the
> >> system police itself. At least that's honest.
> >
> > Wow, that's radical, and interesting.
>
> Actually, to me it sounds a lot like what we did 10 years ago before
> the commitfests except with a way to track the patches (other than the
> mail archives) and more people participating in patch reviews.

Yes, it does remind me of the mbox files I put on the web.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian wrote:
> On Thu, Dec 11, 2014 at 01:12:30PM -0800, Joe Conway wrote:

> > Actually, to me it sounds a lot like what we did 10 years ago before
> > the commitfests except with a way to track the patches (other than the
> > mail archives) and more people participating in patch reviews.
>
> Yes, it does remind me of the mbox files I put on the web.

The whole commitfest process was put in place in part precisely so we
could get rid of your mboxen. Please let's not get back to that!

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut-2 wrote
> On 12/11/14 1:35 AM, Bruce Momjian wrote:
>> I have heard repeated concerns about the commitfest process in the past
>> few months. The fact we have been in a continual commitfest since
>> August also is concerning.
>
> I realized the other day, I'm embracing the idea of a continual
> commitfest.
>
> As far as I'm concerned, we might as well just have one commitfest per
> major release. Call it a patch list. Make the list sortable by created
> date and last-updated date, and let the system police itself. At least
> that's honest.

Having just written about the idea of a CF manager and a CF reviewer...
while having a continual CF works in theory I think breaking it down into,
say, quarters and having each pair commit to 3 months of being in charge has
some logic behind it.

The "patch list" concept should be formalized, and should include a
"targeted release" concept. The idea of a CF seems like it should apply to
the people involved and include an explicit choosing of items from the
"patch list" that are going to get attention by those people during the CF
period. A policy of every new addition being added to the next CF, along
with an item being involved in at least every other CF, would make sense.
Moving along the path of "continuous delivery" maybe just create "CF teams"
instead of a monolithic "CF process".

David J.

--
View this message in context: http://postgresql.nabble.com/Commitfest-problems-tp5830061p5830189.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On 12/11/2014 01:52 PM, Alvaro Herrera wrote:
> Bruce Momjian wrote:
>> On Thu, Dec 11, 2014 at 01:12:30PM -0800, Joe Conway wrote:
>
>>> Actually, to me it sounds a lot like what we did 10 years ago before
>>> the commitfests except with a way to track the patches (other than the
>>> mail archives) and more people participating in patch reviews.
>>
>> Yes, it does remind me of the mbox files I put on the web.
>
> The whole commitfest process was put in place in part precisely so we
> could get rid of your mboxen. Please let's not get back to that!

Well, the CF process was added for 2 reasons:

1) We were losing track of some patches until integration time, when
Bruce suddently found them in the -hackers archives.

2) In order to give the senior committers some time *off* from reviewing
other people's patches. The idea was to spend 3 weeks or so intensively
reviewing others' patches, and then to have a month (or more) *off* from
working on anything but your own stuff.

While the CFs are still doing (1), support for (2) ended sometime in the
9.3 development cycle. Partly this is because current CFMs are not
permitted to take authoritative steps to ensure that the CF ends on
time, and partly it's because of the increase in big complicated patches
which just don't fit into the CF cycle.

Speaking as the originator of commitfests, they were *always* intended
to be a temporary measure, a step on the way to something else like
continuous integration.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Dec 11, 2014 at 1:07 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> As far as I'm concerned, we might as well just have one commitfest per
>> major release. Call it a patch list. Make the list sortable by created
>> date and last-updated date, and let the system police itself. At least
>> that's honest.
>
> Wow, that's radical, and interesting.

Agreed. I don't think it's radical, though - it's just acknowledging
the elephant in the room, which is that the commitfests in a given
cycle are not really distinct at all. I suspect that better tooling
had a lot to do with the success of commitfests, which is more or less
incidental to how they were originally conceived. There is still room
for improvement there.

I'd have to think about it some more, but I'd almost be ready to vote
for formalizing how things actually work in practice given the chance
(i.e. Voting to follow Peter's suggestion).

--
Peter Geoghegan

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Well, the CF process was added for 2 reasons:

> 1) We were losing track of some patches until integration time, when
> Bruce suddently found them in the -hackers archives.

> 2) In order to give the senior committers some time *off* from reviewing
> other people's patches. The idea was to spend 3 weeks or so intensively
> reviewing others' patches, and then to have a month (or more) *off* from
> working on anything but your own stuff.

Right; I was in the middle of composing words to the same effect when
this arrived.

> While the CFs are still doing (1), support for (2) ended sometime in the
> 9.3 development cycle. Partly this is because current CFMs are not
> permitted to take authoritative steps to ensure that the CF ends on
> time, and partly it's because of the increase in big complicated patches
> which just don't fit into the CF cycle.

I don't see why you think CFMs are not "permitted" to close out a CF
when they want to. At least some of the fests have been closed out per
calendar schedule, punting unprocessed patches to the next fest. We've
certainly utterly failed to do that since August, but I think that's
mismanagement.

> Speaking as the originator of commitfests, they were *always* intended
> to be a temporary measure, a step on the way to something else like
> continuous integration.

Really? How would that address (2)? And please note that part of the
problem we're having right now is that senior people are rebelling
against no longer having any agreed-on time off. IMV, goal (2) is
not optional; if you try to force people into continuous review work,
you'll just lose them completely.

regards, tom lane

On 12/11/2014 02:12 PM, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> While the CFs are still doing (1), support for (2) ended sometime in the
>> 9.3 development cycle. Partly this is because current CFMs are not
>> permitted to take authoritative steps to ensure that the CF ends on
>> time, and partly it's because of the increase in big complicated patches
>> which just don't fit into the CF cycle.
>
> I don't see why you think CFMs are not "permitted" to close out a CF
> when they want to. At least some of the fests have been closed out per
> calendar schedule, punting unprocessed patches to the next fest. We've
> certainly utterly failed to do that since August, but I think that's
> mismanagement.

I love how you're willing to accuse Michael of mismangement when you
yourself have *never* run a CF. If I was Michael, I'd quit right now.

Every CFM who has taken forceful measures to close out the CF on time
has gotten a large amount of flack for it, and absolutely zero back-up
from the committers or the core team. This is why David, Robert and I
will no longer manage CFs (and probably others as well). The CFM is
expected to miraculously end the CF on time, without bouncing or
forwarding unreviewed patches, cutting off patches which already had one
round of review, bouncing out or holding patches from contributors who
don't participate in the review process, nagging anyone too much, or
taking any other steps which anyone might take exception to.

In fact, the technique you cite (just punting the patches to the next
CF) resulted in Fetter getting a great deal of hassling on this list
when he did it. The only people who backed him up on it were other
former CFMs. And I closed out my CF on my expected schedule (based on
number of patches), but only after taking so much shit for it, I'm never
doing it again. And I have a pretty high tolerance for taking shit.

So now you're left with CFMs who won't do anything to wind up the CF
because they know that the committers and hackers will *not* support
them if they do. So, eternal CF.

How about *you* run the next one, Tom?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> How about *you* run the next one, Tom?

I think the limited amount of time I can put into a commitfest is better
spent on reviewing patches than on managing the process.

regards, tom lane

On Fri, Dec 12, 2014 at 7:27 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> On 12/11/2014 02:12 PM, Tom Lane wrote:
> > Josh Berkus <josh(at)agliodbs(dot)com> writes:
> >> While the CFs are still doing (1), support for (2) ended sometime in the
> >> 9.3 development cycle. Partly this is because current CFMs are not
> >> permitted to take authoritative steps to ensure that the CF ends on
> >> time, and partly it's because of the increase in big complicated patches
> >> which just don't fit into the CF cycle.
> >
> > I don't see why you think CFMs are not "permitted" to close out a CF
> > when they want to. At least some of the fests have been closed out per
> > calendar schedule, punting unprocessed patches to the next fest. We've
> > certainly utterly failed to do that since August, but I think that's
> > mismanagement.
>
> I love how you're willing to accuse Michael of mismangement when you
> yourself have *never* run a CF. If I was Michael, I'd quit right now.
>

Er, just to be clear, I was basically not the CF manager for the one that
began in October. I just showed up because I had some hours left and things
were not moving on for many patches. Now, and I think that nobody is
volunteering, I am fine to be the next CF manager for the one beginning on
Monday. That would help making things move on.
Regards,
--
Michael

On Fri, Dec 12, 2014 at 7:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Josh Berkus <josh(at)agliodbs(dot)com> writes:
> > How about *you* run the next one, Tom?
>
> I think the limited amount of time I can put into a commitfest is better
> spent on reviewing patches than on managing the process.
>
That's not only your case, I have the same feeling that time is more
productive when being focused on a single task. CFM work take your energy
away from other things, and if a CFM reviews patches he'd tend to miss
things in the patches he looked at.
--
Michael

On Thu, Dec 11, 2014 at 5:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> How about *you* run the next one, Tom?
>
> I think the limited amount of time I can put into a commitfest is better
> spent on reviewing patches than on managing the process.

That's not really the point. The point is that managing the last
CommitFest in particular is roughly equivalent to having your arm
boiled in hot oil. A certain percentage of the people whose patches
are obviously not ready to commit complain and moan about how (a)
their patch really is ready for prime-time, despite all appearances to
the contrary, and/or (b) their patch is so important that it deserves
and exception, and/or (c) how you are a real jerk for treating them so
unfairly. This is not fun, which is why I've given up on doing it. I
could not get a single person to support me when I tried to enforce
any scheduling discipline, so my conclusion was that the community did
not care about hitting the schedule; and it took weeks of 24x7 effort
to build a consensus to reject even one large, problematic patch whose
author wasn't willing to admit defeat. If the community is prepared
to invest some trusted individuals with real authority, then we might
be able to remove some of the pain here, but when that was discussed
at a PGCon developer meeting a few years back, it was clear that no
more than 20% of the people in the room were prepared to support that
concept.

At this point, though, I'm not sure how much revisiting that
discussion would help. I think the problem we need to solve here is
that there are just not enough senior people with an adequate amount
of time to review. Whether it's because the patches are more complex
or that there are more of them or that those senior people have become
less available due to other commitments, we still need more senior
people involved to be able to handle the patches we've got in a timely
fashion without unduly compromising stability. And we also need to do
a better job recruiting and retaining mid-level reviewers, both
because that's where senior people eventually come from, and because
it reduces the load on the senior people we've already got.

(I note that the proposal to have the CFM review everything is merely
one way of meeting the need to have senior people spend more time
reviewing. But I assure all of you that I spend as much time
reviewing as I can find time for. If someone wants to pay me the same
salary I'm making now to do nothing but review patches, I'll think
about it. But even then, that would also mean that I wasn't spending
time writing patches of my own.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:

> (I note that the proposal to have the CFM review everything is merely
> one way of meeting the need to have senior people spend more time
> reviewing. But I assure all of you that I spend as much time
> reviewing as I can find time for. If someone wants to pay me the same
> salary I'm making now to do nothing but review patches, I'll think
> about it. But even then, that would also mean that I wasn't spending
> time writing patches of my own.)

I have heard the idea of a "cross-company PostgreSQL foundation" of some
sort that would hire a developer just to manage commitfests, do patch
reviews, apply bugfixes, etc, without the obligations that come from
individual companies' schedules for particular development roadmaps,
customer support, and the like. Of course, only a senior person would
be able to fill this role because it requires considerable experience.

Probably this person should be allowed to work on their own patches if
they so desire; otherwise there is a risk that experience dilutes.
Also, no single company should dictate what this person's priorities
are, other than general guidelines: general stability, submitted patches
get attention, bugs get closed, releases get out, coffee gets brewed.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Dec 12, 2014 at 9:15 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> (I note that the proposal to have the CFM review everything is merely
>> one way of meeting the need to have senior people spend more time
>> reviewing. But I assure all of you that I spend as much time
>> reviewing as I can find time for. If someone wants to pay me the same
>> salary I'm making now to do nothing but review patches, I'll think
>> about it. But even then, that would also mean that I wasn't spending
>> time writing patches of my own.)
>
> I have heard the idea of a "cross-company PostgreSQL foundation" of some
> sort that would hire a developer just to manage commitfests, do patch
> reviews, apply bugfixes, etc, without the obligations that come from
> individual companies' schedules for particular development roadmaps,
> customer support, and the like. Of course, only a senior person would
> be able to fill this role because it requires considerable experience.
>
> Probably this person should be allowed to work on their own patches if
> they so desire; otherwise there is a risk that experience dilutes.
> Also, no single company should dictate what this person's priorities
> are, other than general guidelines: general stability, submitted patches
> get attention, bugs get closed, releases get out, coffee gets brewed.

Yeah, that would be great, and even better if we could get 2 or 3
positions funded so that the success or failure isn't too much tied to
a single individual. But even getting 1 position funded in a
stable-enough fashion that someone would be willing to bet on it seems
like a challenge. (Maybe other people here are less risk-averse than
I am.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Dec 11, 2014 at 05:55:56PM -0500, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
> > How about *you* run the next one, Tom?
>
> I think the limited amount of time I can put into a commitfest is
> better spent on reviewing patches than on managing the process.

With utmost respect, Tom, you seem to carve off an enormous amount of
time to follow -bugs and -general. What say you unsubscribe to those
lists for the duration of your tenure as CFM?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 2014-12-12 07:10:40 -0800, David Fetter wrote:
> On Thu, Dec 11, 2014 at 05:55:56PM -0500, Tom Lane wrote:
> > Josh Berkus <josh(at)agliodbs(dot)com> writes:
> > > How about *you* run the next one, Tom?
> >
> > I think the limited amount of time I can put into a commitfest is
> > better spent on reviewing patches than on managing the process.
>
> With utmost respect,

FWIW, the way you frequently use this phrase doesn't come over as
actually being respectful.

> Tom, you seem to carve off an enormous amount of
> time to follow -bugs and -general. What say you unsubscribe to those
> lists for the duration of your tenure as CFM?

And why on earth would that be a good idea? These bugs need to be fixed
- we're actually behind on that front. Are we now really trying to
dictate how other developers manage their time? It's one thing to make
up rules that say "one review for one commit" or something, it's
something entirely else to try to assign tasks to them.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Dec 12, 2014 at 9:15 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> Robert Haas wrote:
>>> (I note that the proposal to have the CFM review everything is merely
>>> one way of meeting the need to have senior people spend more time
>>> reviewing. But I assure all of you that I spend as much time
>>> reviewing as I can find time for. If someone wants to pay me the same
>>> salary I'm making now to do nothing but review patches, I'll think
>>> about it. But even then, that would also mean that I wasn't spending
>>> time writing patches of my own.)

>> I have heard the idea of a "cross-company PostgreSQL foundation" of some
>> sort that would hire a developer just to manage commitfests, do patch
>> reviews, apply bugfixes, etc, without the obligations that come from
>> individual companies' schedules for particular development roadmaps,
>> customer support, and the like. Of course, only a senior person would
>> be able to fill this role because it requires considerable experience.

> Yeah, that would be great, and even better if we could get 2 or 3
> positions funded so that the success or failure isn't too much tied to
> a single individual. But even getting 1 position funded in a
> stable-enough fashion that someone would be willing to bet on it seems
> like a challenge. (Maybe other people here are less risk-averse than
> I am.)

Yeah, it would be hard to sell anyone on that unless the foundation
was so well funded that it could clearly afford to keep paying you
for years into the future.

I'm not really on board with the CFM-reviews-everything idea anyway.
I don't think that can possibly work well, because it supposes that senior
reviewers are interchangeable, which they aren't. Everybody's got pieces
of the system that they know better than other pieces.

Also, one part of the point of the review mechanism is that it's supposed
to provide an opportunity for less-senior reviewers to look at parts of
the code that they maybe don't know so well, and thereby help grow them
into senior people. If we went over to the notion of some one (or a few)
senior people doing all the reviewing, it might make the review process
more expeditious but it would lose the training aspect. Of course, maybe
the training aspect was never worth anything; I'm not in a position to
opine on that. But I don't really think that centralizing that
responsibility would be a good thing in the long run.

regards, tom lane

On Fri, Dec 12, 2014 at 10:50:56AM -0500, Tom Lane wrote:
> Also, one part of the point of the review mechanism is that it's supposed
> to provide an opportunity for less-senior reviewers to look at parts of
> the code that they maybe don't know so well, and thereby help grow them
> into senior people. If we went over to the notion of some one (or a few)
> senior people doing all the reviewing, it might make the review process
> more expeditious but it would lose the training aspect. Of course, maybe
> the training aspect was never worth anything; I'm not in a position to
> opine on that. But I don't really think that centralizing that
> responsibility would be a good thing in the long run.

That is a very good point --- we have certainly had people doing reviews
long enough to know if the review process is preparing developers for
more complex tasks. I don't know the answer myself, which might say
something.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Fri, Dec 12, 2014 at 04:21:43PM +0100, Andres Freund wrote:
> On 2014-12-12 07:10:40 -0800, David Fetter wrote:
> > On Thu, Dec 11, 2014 at 05:55:56PM -0500, Tom Lane wrote:
> > > Josh Berkus <josh(at)agliodbs(dot)com> writes:
> > > > How about *you* run the next one, Tom?
> > >
> > > I think the limited amount of time I can put into a commitfest is
> > > better spent on reviewing patches than on managing the process.
> >
> > With utmost respect,
>
> FWIW, the way you frequently use this phrase doesn't come over as
> actually being respectful.

Respect is quantified, and in this case, the most afforded is the most
earned. In the case of criticizing the work of others without an
offer of help them do it better, respect for that behavior does have
some pretty sharp upper limits, so yes, utmost is in that context.

> > Tom, you seem to carve off an enormous amount of time to follow
> > -bugs and -general. What say you unsubscribe to those lists for
> > the duration of your tenure as CFM?
>
> And why on earth would that be a good idea?

Because Tom Lane is not the person whose time is best spent screening
these mailing lists.

> These bugs need to be fixed - we're actually behind on that front.

So you're proposing a bug triage system, which is a separate
discussion. Let's have that one in a separate thread.

> Are we now really trying to dictate how other developers manage
> their time?

I was merely pointing out that time can be allocated, and that it
appeared it could be allocated from a bucket for which persons less
knowledgeable--perhaps a good bit less knowledgeable--about the entire
code base than Tom are well suited.

> It's one thing to make up rules that say "one review for one commit"
> or something,

And how do you think that would work out. Are you up for following
it?

> it's something entirely else to try to assign tasks to them.

I was, as I mentioned, merely pointing out that trade-offs are
available.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 12/12/2014 06:30 AM, Robert Haas wrote:
> Yeah, that would be great, and even better if we could get 2 or 3
> positions funded so that the success or failure isn't too much tied to
> a single individual. But even getting 1 position funded in a
> stable-enough fashion that someone would be willing to bet on it seems
> like a challenge. (Maybe other people here are less risk-averse than
> I am.)

Well, first, who would that person be? Last I checked, all of the senior
committers were spoken for. I like this idea, but the list of people
who could fill the role is pretty short, and I couldn't possibly start
fundraising unless I had a candidate.

Second, I don't think someone's employment will make a difference in
fixing the commitfest and patch review *process* unless the contributors
agree that it needs fixing and that they are willing to make changes to
their individual workflow to fix it. Right now there is no consensus
about moving forward in our patch review process; everyone seems to want
the problem to go away without changing anything.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 12.12.2014 19:07, Bruce Momjian wrote:
> On Fri, Dec 12, 2014 at 10:50:56AM -0500, Tom Lane wrote:
>> Also, one part of the point of the review mechanism is that it's
>> supposed to provide an opportunity for less-senior reviewers to
>> look at parts of the code that they maybe don't know so well, and
>> thereby help grow them into senior people. If we went over to the
>> notion of some one (or a few) senior people doing all the
>> reviewing, it might make the review process more expeditious but it
>> would lose the training aspect. Of course, maybe the training
>> aspect was never worth anything; I'm not in a position to opine on
>> that. But I don't really think that centralizing that
>> responsibility would be a good thing in the long run.
>
> That is a very good point --- we have certainly had people doing
> reviews long enough to know if the review process is preparing
> developers for more complex tasks. I don't know the answer myself,
> which might say something.

I can't speak for the others, but for me it certainly is a useful way to
learn new stuff. Maybe not as important as working on my own patches,
but it usually forces me to learn something new, and gives me a
different perspective.

regards
Tomas

On 12 December 2014 at 15:10, David Fetter <david(at)fetter(dot)org> wrote:
> On Thu, Dec 11, 2014 at 05:55:56PM -0500, Tom Lane wrote:
>> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> > How about *you* run the next one, Tom?
>>
>> I think the limited amount of time I can put into a commitfest is
>> better spent on reviewing patches than on managing the process.

IIRC Tom was pretty much the only person doing patch review for
probably 5 years during 2003-2008, maybe others. AFAICS he was
managing that process. Thank you, Tom.

I've never seen him moan loudly about this, so I'm surprised to hear
such things from people that have done much less.

Any solution to our current problems will come from working together,
not by fighting.

We just need to do more reviews. Realising this, I have begun to do
more. I encourage others to do this also.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/11/2014 02:55 PM, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> How about *you* run the next one, Tom?
>
> I think the limited amount of time I can put into a commitfest is better
> spent on reviewing patches than on managing the process.

Agreed but

That means committers/hackers have to suck it up when the manager closes
the commit fest.

We don't get our cake and eat it too. We either accept that the CFM has
the authority to do exactly what they are supposed to do, or we don't.
JD

--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, @cmdpromptinc
"If we send our children to Caesar for their education, we should
not be surprised when they come back as Romans."

On 12/12/2014 06:30 AM, Robert Haas wrote:

> Yeah, that would be great, and even better if we could get 2 or 3
> positions funded so that the success or failure isn't too much tied to
> a single individual. But even getting 1 position funded in a
> stable-enough fashion that someone would be willing to bet on it seems
> like a challenge. (Maybe other people here are less risk-averse than
> I am.)

We (not CMD, the community) with proper incentive could fund this. It
really wouldn't be that hard. That said, there would have to be a clear
understanding of expectations, results, and authority.

JD

>

--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, @cmdpromptinc
"If we send our children to Caesar for their education, we should
not be surprised when they come back as Romans."

On 12/12/2014 10:59 AM, Simon Riggs wrote:
>
> On 12 December 2014 at 15:10, David Fetter <david(at)fetter(dot)org> wrote:
>> On Thu, Dec 11, 2014 at 05:55:56PM -0500, Tom Lane wrote:
>>> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>>>> How about *you* run the next one, Tom?
>>>
>>> I think the limited amount of time I can put into a commitfest is
>>> better spent on reviewing patches than on managing the process.
>
> IIRC Tom was pretty much the only person doing patch review for
> probably 5 years during 2003-2008, maybe others. AFAICS he was
> managing that process. Thank you, Tom.
>
> I've never seen him moan loudly about this, so I'm surprised to hear
> such things from people that have done much less.
>
> Any solution to our current problems will come from working together,
> not by fighting.
>
> We just need to do more reviews. Realising this, I have begun to do
> more. I encourage others to do this also.
>

Simon,

Well said but again, I think a lot of people are hand waving about a
simple problem (within the current structure) and that problem is just
one of submission.

Those doing the patch review/writing need to submit to the authority of
the CFM or CFC (commit fest comittee). That happens and a lot of the
angst around this process goes away.

Sincerely,

Joshua D. Drake

--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, @cmdpromptinc
"If we send our children to Caesar for their education, we should
not be surprised when they come back as Romans."

Joshua D. Drake wrote:
>
> On 12/12/2014 06:30 AM, Robert Haas wrote:
>
> >Yeah, that would be great, and even better if we could get 2 or 3
> >positions funded so that the success or failure isn't too much tied to
> >a single individual. But even getting 1 position funded in a
> >stable-enough fashion that someone would be willing to bet on it seems
> >like a challenge. (Maybe other people here are less risk-averse than
> >I am.)
>
> We (not CMD, the community) with proper incentive could fund this. It really
> wouldn't be that hard. That said, there would have to be a clear
> understanding of expectations, results, and authority.

Uh, really? Last I looked at the numbers from SPI treasurer reports,
they are not impressive enough to hire a full-time engineer, let alone a
senior one.

The Linux Foundation has managed to pay for Linus Torvalds somehow, so
it does sound possible. We have a number of companies making money all
over the globe, at least.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/12/2014 11:35 AM, Alvaro Herrera wrote:
> Uh, really? Last I looked at the numbers from SPI treasurer reports,
> they are not impressive enough to hire a full-time engineer, let alone a
> senior one.
>
> The Linux Foundation has managed to pay for Linus Torvalds somehow, so
> it does sound possible. We have a number of companies making money all
> over the globe, at least.

You're looking at this wrong. We have that amount of money in the
account based on zero fundraising whatsoever, which we don't do because
we don't spend the money. We get roughly $20,000 per year just by
putting up a "donate" link, and not even promoting it.

So, what this would take is:

1) a candidate who is currently a known major committer

2) clear goals for what this person would spend their time doing

3) buy-in from the Core Team, the committers, and the general hackers
community (including buy-in to the idea of favorable publicity for
funding supporters)

4) an organizing committee with the time to deal with managing
foundation funds

If we had those four things, the fundraising part would be easy. I
speak as someone who used to raise $600,000 per year for a non-profit in
individual gifts alone.

However, *I'm* not clear on what problems this non-profit employed
person would be solving for the community. I doubt anyone else is
either. Until we have consensus on that, there's no point in talking
about anything else.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 11.12.2014 16:06, Bruce Momjian wrote:
> On Wed, Dec 10, 2014 at 11:00:21PM -0800, Josh Berkus wrote:
>>
>> I will add:
>>
>> 4. commitfest managers have burned out and refuse to do it again
>
> Agreed. The "fun", if it was ever there, has left the commitfest
> process.

I've never been a CFM, but from my experience as a patch author and
reviewer, I think there are two or three reasons for that (of course,
I'm not saying those are the most important ones or the only ones):

1) unclear definition of what CFM is expected to do

The current wiki page describing the role of CFM [1] is rather
obsolete, IMHO. For example it says that CFM assigns patches ro
reviewers, posts announcements to pgsql-rrreviewers, etc.

I don't think this was really followed in recent CFs.

This however results in people filling the gaps with what they
believe the CFM should do, causing misunderstandings etc. Shall
we update the description a bit, to reflect the current state
of affairs?

Maybe we should also consider which responsibilities should be
shifted back to the developers and reviewers. E.g. do we really
expect the CFM to assign patches to reviewers?

2) not really following the rules

We do have a few rules that we don't follow as much as we should,
notably:

* 1:1 for patches:reviews (one review for each submitted patch)
* no new patches after the CF starts (post it to the next one)
* CF ends at a specific date

I believe violating those rules is related to (1) because it may
lead to perception that CFM makes them up or does not enforce them
equally for all patches.

3) manual processing that could be automated

I think the CF site was a huge step forward, but maybe we could
improve it, to automate some of the CFM tasks? For example
integrating it a bit more tightly with the mailinglist (which would
make the life easier even for patch authors and reviewers)?

However as I said before, I never was a CFM - I'd like to hear from the
actual CFMs what's their opinion on this.

kind regards
Tomas

[1] https://wiki.postgresql.org/wiki/Running_a_CommitFest

On 12/12/2014 11:35 AM, Alvaro Herrera wrote:

>> We (not CMD, the community) with proper incentive could fund this. It really
>> wouldn't be that hard. That said, there would have to be a clear
>> understanding of expectations, results, and authority.
>
> Uh, really?

Yeah I think so. Money can be easy to get, when clear leadership and
goals are presented.

> Last I looked at the numbers from SPI treasurer reports,
> they are not impressive enough to hire a full-time engineer, let alone a
> senior one.

1. We don't need a full-time engineer to manage a commitfest. We need a
manager or PM.

2. The original idea came from cross-company (which is part of the
community)

3. There is more non-profits in this game than just SPI

4. We could do it on a 6 month for 1 year contract

Sincerely,

Joshua D. Drake

--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, @cmdpromptinc
"If we send our children to Caesar for their education, we should
not be surprised when they come back as Romans."

On Fri, Dec 12, 2014 at 8:43 PM, Tomas Vondra <tv(at)fuzzy(dot)cz> wrote:
> On 11.12.2014 16:06, Bruce Momjian wrote:
>> On Wed, Dec 10, 2014 at 11:00:21PM -0800, Josh Berkus wrote:
> 3) manual processing that could be automated
>
> I think the CF site was a huge step forward, but maybe we could
> improve it, to automate some of the CFM tasks? For example
> integrating it a bit more tightly with the mailinglist (which would
> make the life easier even for patch authors and reviewers)?

Just as a note abot this one part along (I'll read the rest later). I
do have the new version of the CF app more or less ready to deploy,
but I got bogged down by thinking "I'll do it between two commitfests
to not be disruptive". But there has been no "between two
commitfests". Hopefully I can get around to doing it during the
holidays. It does integrate much tighter with the archives, that's
probably the core feature of it.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 12/12/2014 11:52 AM, Magnus Hagander wrote:
> On Fri, Dec 12, 2014 at 8:43 PM, Tomas Vondra <tv(at)fuzzy(dot)cz> wrote:
>> On 11.12.2014 16:06, Bruce Momjian wrote:
>>> On Wed, Dec 10, 2014 at 11:00:21PM -0800, Josh Berkus wrote:
>> 3) manual processing that could be automated
>>
>> I think the CF site was a huge step forward, but maybe we could
>> improve it, to automate some of the CFM tasks? For example
>> integrating it a bit more tightly with the mailinglist (which would
>> make the life easier even for patch authors and reviewers)?
>
> Just as a note abot this one part along (I'll read the rest later). I
> do have the new version of the CF app more or less ready to deploy,
> but I got bogged down by thinking "I'll do it between two commitfests
> to not be disruptive". But there has been no "between two
> commitfests". Hopefully I can get around to doing it during the
> holidays. It does integrate much tighter with the archives, that's
> probably the core feature of it.

It also automates a bunch of the emailing no?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Fri, Dec 12, 2014 at 9:05 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 12/12/2014 11:52 AM, Magnus Hagander wrote:
>> On Fri, Dec 12, 2014 at 8:43 PM, Tomas Vondra <tv(at)fuzzy(dot)cz> wrote:
>>> On 11.12.2014 16:06, Bruce Momjian wrote:
>>>> On Wed, Dec 10, 2014 at 11:00:21PM -0800, Josh Berkus wrote:
>>> 3) manual processing that could be automated
>>>
>>> I think the CF site was a huge step forward, but maybe we could
>>> improve it, to automate some of the CFM tasks? For example
>>> integrating it a bit more tightly with the mailinglist (which would
>>> make the life easier even for patch authors and reviewers)?
>>
>> Just as a note abot this one part along (I'll read the rest later). I
>> do have the new version of the CF app more or less ready to deploy,
>> but I got bogged down by thinking "I'll do it between two commitfests
>> to not be disruptive". But there has been no "between two
>> commitfests". Hopefully I can get around to doing it during the
>> holidays. It does integrate much tighter with the archives, that's
>> probably the core feature of it.
>
> It also automates a bunch of the emailing no?

Yes.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Fri, Dec 12, 2014 at 12:30 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> It also automates a bunch of the emailing no?
>
> Yes.

Please let me know the details (privately or otherwise). I'd like to
try it out again.

--
Peter Geoghegan

>>> Just as a note abot this one part along (I'll read the rest later). I
>>> do have the new version of the CF app more or less ready to deploy,
>>> but I got bogged down by thinking "I'll do it between two commitfests
>>> to not be disruptive". But there has been no "between two
>>> commitfests". Hopefully I can get around to doing it during the
>>> holidays. It does integrate much tighter with the archives, that's
>>> probably the core feature of it.
>>
>> It also automates a bunch of the emailing no?
>
> Yes.

I can key in a bunch of the backlog of patches into the new app over the
holidays, but not before then.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 12/12/14, 2:38 PM, Josh Berkus wrote:
>
>>>> Just as a note abot this one part along (I'll read the rest later). I
>>>> do have the new version of the CF app more or less ready to deploy,
>>>> but I got bogged down by thinking "I'll do it between two commitfests
>>>> to not be disruptive". But there has been no "between two
>>>> commitfests". Hopefully I can get around to doing it during the
>>>> holidays. It does integrate much tighter with the archives, that's
>>>> probably the core feature of it.
>>>
>>> It also automates a bunch of the emailing no?
>>
>> Yes.
>
> I can key in a bunch of the backlog of patches into the new app over the
> holidays, but not before then.

FWIW, I suspect a call for help on -general or IRC would find volunteers for any necessary data entry work...
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 12/12/14, 1:44 PM, Joshua D. Drake wrote:
> 1. We don't need a full-time engineer to manage a commitfest. We need a manager or PM.

I don't think that's actually true. The major points on this thread are that 1) we don't have enough capacity for doing reviews and 2) the CFM has no authority to enforce anything.

I see no way that #2 can be addressed by a mere manager/PM. If three *very* senior community members (David, Josh and Robert) couldn't get this done there's no way a PM could. (Well, I suppose of Tom was standing behind them with a flaming sword it might work...)

Even so, this still wouldn't address the real problem, which is lack of review capacity.

FWIW, I faced the same problem at Enova: good, solid reviews were very important for maintaining the quality of the data and database code, yet were a constant source of pain and friction. And that was with people being paid to do them, as well as a very extensive set of unit tests.

In other words, this isn't an easy problem to solve.

One thing that I think would help is modifying the CF app to allow for multiple reviewers. IIRC I reviewed 4 or 5 patches but I didn't mark myself as reviewer of any of them because I don't feel I have enough knowledge to fulfill that role.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 12/12/2014 01:22 PM, Jim Nasby wrote:
>
> On 12/12/14, 1:44 PM, Joshua D. Drake wrote:
>> 1. We don't need a full-time engineer to manage a commitfest. We need
>> a manager or PM.
>
> I don't think that's actually true. The major points on this thread are
> that 1) we don't have enough capacity for doing reviews and 2) the CFM
> has no authority to enforce anything.
>
> I see no way that #2 can be addressed by a mere manager/PM. If three
> *very* senior community members (David, Josh and Robert) couldn't get
> this done there's no way a PM could. (Well, I suppose of Tom was
> standing behind them with a flaming sword it might work...)

#2 is solved by my previous comments about giving the CFM/C the
authority. -Core could do that, they are in charge of release.

#1 is a more difficult problem for sure.

JD
--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, @cmdpromptinc
"If we send our children to Caesar for their education, we should
not be surprised when they come back as Romans."

On Sat, Dec 13, 2014 at 5:30 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Fri, Dec 12, 2014 at 9:05 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> On 12/12/2014 11:52 AM, Magnus Hagander wrote:
>>> On Fri, Dec 12, 2014 at 8:43 PM, Tomas Vondra <tv(at)fuzzy(dot)cz> wrote:
>>>> On 11.12.2014 16:06, Bruce Momjian wrote:
>>>>> On Wed, Dec 10, 2014 at 11:00:21PM -0800, Josh Berkus wrote:
>>>> 3) manual processing that could be automated
>>>>
>>>> I think the CF site was a huge step forward, but maybe we could
>>>> improve it, to automate some of the CFM tasks? For example
>>>> integrating it a bit more tightly with the mailinglist (which would
>>>> make the life easier even for patch authors and reviewers)?
>>>
>>> Just as a note abot this one part along (I'll read the rest later). I
>>> do have the new version of the CF app more or less ready to deploy,
>>> but I got bogged down by thinking "I'll do it between two commitfests
>>> to not be disruptive". But there has been no "between two
>>> commitfests". Hopefully I can get around to doing it during the
>>> holidays. It does integrate much tighter with the archives, that's
>>> probably the core feature of it.
>>
>> It also automates a bunch of the emailing no?
>
> Yes.
That's pretty cool.
--
Michael

On Sat, Dec 13, 2014 at 1:13 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
> On 12/12/2014 11:35 AM, Alvaro Herrera wrote:
> > Uh, really? Last I looked at the numbers from SPI treasurer reports,
> > they are not impressive enough to hire a full-time engineer, let alone a
> > senior one.
> >
> > The Linux Foundation has managed to pay for Linus Torvalds somehow, so
> > it does sound possible. We have a number of companies making money all
> > over the globe, at least.
>
> You're looking at this wrong. We have that amount of money in the
> account based on zero fundraising whatsoever, which we don't do because
> we don't spend the money. We get roughly $20,000 per year just by
> putting up a "donate" link, and not even promoting it.
>
> So, what this would take is:
>
> 1) a candidate who is currently a known major committer
>
> 2) clear goals for what this person would spend their time doing
>
> 3) buy-in from the Core Team, the committers, and the general hackers
> community (including buy-in to the idea of favorable publicity for
> funding supporters)
>
> 4) an organizing committee with the time to deal with managing
> foundation funds
>
> If we had those four things, the fundraising part would be easy. I
> speak as someone who used to raise $600,000 per year for a non-profit in
> individual gifts alone.
>
> However, *I'm* not clear on what problems this non-profit employed
> person would be solving for the community.

I am not sure if employing the person for full-time can be beneficial
in the short to medium term and how easy or difficult it would be
to convince a senior guy for this kind of job at this point when there
is no such fund available or appropriate mechanism to collect the same
in place, however if the community would come up with a scheme such
that they can pay ~$7,000 per commit fest (which means ~$30000 to
~$35,000 per year) to manage and run that commit fest, then we might
get some volunteers or ask some people (who can do the justice to this
work) for this work.
For managing the commit fest that particular person don't have to give-up
his current job, rather he has to get some buy-in from his employer that
for that period of time he has to be more involved in community work
without compromising much on his day-to-day work. Now I think
such a person should not only be doing the pure management of CF,
but doing the review of some of the important patches, something like
previously done by Heikki or Robert or some other senior members.
I am not really sure will such a money be lucrative enough for some
senior person or if we can raise such a money, but surely from where
I am sitting I could visualize it as a win-win situation where community
doesn't have to pay too high money and the person chosen don't have
to compromise too much on his usual work and I think this could get
the major benefit we are looking at this stage. I understand for long
term we might need something better than what I am proposing, but
for short to medium term such a scheme might prove to be a good deal
for every one.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 12/12/2014 06:02 AM, Josh Berkus wrote:
>
> Speaking as the originator of commitfests, they were *always* intended
> to be a temporary measure, a step on the way to something else like
> continuous integration.

I'd really like to see the project revisit some of the underlying
assumptions that're being made in this discussion:

- Patches must be email attachments to a mailing list

- Changes must be committed by applying a diff

... and take a look at some of the options a git-based workflow might
offer, especially in combination with some of the tools out there that
help track working branches, run CI, etc.

Having grown used to push/pull workflows with CI integration I find the
PostgreSQL patch workflow very frustrating, especially for larger
patches. It's particularly annoying to see a patch series squashed into
a monster patch whenever it changes hands or gets rebased, because it's
being handed around as a great honking diff not a git working branch.

Is it time to stop using git like CVS?

(/hides)

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/12/2014 06:01 AM, David G Johnston wrote:
> The "patch list" concept should be formalized, and should include a
> "targeted release" concept.

IMO, the "patch list" concept should be discarded in favour of a
"working tree list".

At this point, given the challenges the CF process faces, I can't say
I'd be sad if Pg adopted GitHub's push/pull process on their
infrastructure ... but I'm aware that's not going to happen, and I
understand at least some of the reasons why.

I'd be really happy to see the CF somewhat closer to that than a bunch
of message-id copy-and-pasting and patch emailing though.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/12/14 20:43, Josh Berkus wrote:
> On 12/12/2014 11:35 AM, Alvaro Herrera wrote:
>> Uh, really? Last I looked at the numbers from SPI treasurer reports,
>> they are not impressive enough to hire a full-time engineer, let alone a
>> senior one.
>>
>> The Linux Foundation has managed to pay for Linus Torvalds somehow, so
>> it does sound possible. We have a number of companies making money all
>> over the globe, at least.

I think Álvaro (Herrera) got to the real point when he suggested to
fund a developer. Or three, as was also suggested. But to really nail it
down, I'd say not only for CFM. I think, overall, the PostgreSQL
community would really need to seriously consider raising funds and pay
with that full time development for some senior developers.

That would also allow to have development more oriented into what
the community really wants, rather than what other companies or
individuals work for (which is absolutely great, of course).

> You're looking at this wrong. We have that amount of money in the
> account based on zero fundraising whatsoever, which we don't do because
> we don't spend the money. We get roughly $20,000 per year just by
> putting up a "donate" link, and not even promoting it.
>
> So, what this would take is:
>
> 1) a candidate who is currently a known major committer

I think it would be even better to sell this approach as a
long-term strategy, not tied to any particular candidate. Sure, some
known major committer is for sure a good selling point; but a well
communicated strategy for a long-term foundation-like fund raising to
improve PostgreSQL in certain ways is the way to go.
>
> 2) clear goals for what this person would spend their time doing
+1. That may be the Core Team based on feedback/input from all the
list, or something like that
>
> 3) buy-in from the Core Team, the committers, and the general hackers
> community (including buy-in to the idea of favorable publicity for
> funding supporters)
+1
>
> 4) an organizing committee with the time to deal with managing
> foundation funds
+10000. Absolutely necessary, otherwise funding will not work
>
> If we had those four things, the fundraising part would be easy. I
> speak as someone who used to raise $600,000 per year for a non-profit in
> individual gifts alone.

I know it sounds difficult, and surely it is, but I believe the
PostgreSQL community should be able to raise, globally, some millions
per year to stably, and permanently, fund this community-guided
development and have our best developers devoted 100% to PostgreSQL.

Regards,

Álvaro

--
Álvaro Hernández Tortosa

-----------
8Kdata

On 13/12/14 22:37, Craig Ringer wrote:
> On 12/12/2014 06:02 AM, Josh Berkus wrote:
>>
>> Speaking as the originator of commitfests, they were *always* intended
>> to be a temporary measure, a step on the way to something else like
>> continuous integration.
>
> I'd really like to see the project revisit some of the underlying
> assumptions that're being made in this discussion:
>
> - Patches must be email attachments to a mailing list
>
> - Changes must be committed by applying a diff
>
> ... and take a look at some of the options a git-based workflow might
> offer, especially in combination with some of the tools out there that
> help track working branches, run CI, etc.
>
> Having grown used to push/pull workflows with CI integration I find the
> PostgreSQL patch workflow very frustrating, especially for larger
> patches. It's particularly annoying to see a patch series squashed into
> a monster patch whenever it changes hands or gets rebased, because it's
> being handed around as a great honking diff not a git working branch.
>
> Is it time to stop using git like CVS?
>
> (/hides)
>

Having dealt with other projects that use a more git centric + CI
approach, I can say that trying to do reviews can be just frustrating in
that case too:

- quirky and annoying web interfaces
- changesets "expiring" in the middle of you reviewing them
- pulls and rebases making actually making it harder to see what was
changed in new changeset versions

I think the basic issue is that reviewing is hard, and while our
system-ismation of the workflow is really primitive, and could be much
better (that seems to be being worked on), the *tool* is not really the
problem.

regards

Mark

On 13/12/14 09:37, Craig Ringer wrote:

>> Speaking as the originator of commitfests, they were *always* intended
>> to be a temporary measure, a step on the way to something else like
>> continuous integration.
>
> I'd really like to see the project revisit some of the underlying
> assumptions that're being made in this discussion:
>
> - Patches must be email attachments to a mailing list
>
> - Changes must be committed by applying a diff
>
> ... and take a look at some of the options a git-based workflow might
> offer, especially in combination with some of the tools out there that
> help track working branches, run CI, etc.
>
> Having grown used to push/pull workflows with CI integration I find the
> PostgreSQL patch workflow very frustrating, especially for larger
> patches. It's particularly annoying to see a patch series squashed into
> a monster patch whenever it changes hands or gets rebased, because it's
> being handed around as a great honking diff not a git working branch.
>
> Is it time to stop using git like CVS?

While not so active these days with PostgreSQL, I believe there is still
great scope for improving the workflow of reviewing and applying
patches. And while the commitfests were a good idea when they started,
it's obvious that in their current form they do not scale to meet the
current rate of development.

If I could name just one thing that I think would improve things it
would be submission of patches to the list in git format-patch format.
Why? Because it enables two things: 1) by definition patches are
"small-chunked" into individually reviewable pieces and 2) subject lines
allow list members to filter things that aren't relevant to them.

Here's an example of how the workflow works for the QEMU project which
I'm involved in, which also has similar issues in terms of numbers of
developers and supported platforms:

1) Developer submits a patch to the -devel list with git-format-patch

- For subsystems with listed maintainers in the MAINTAINERS file, the
maintainer must be CCd on the patchset (this is so that even if
developers decide to filter PATCH emails to the list, they still get the
CC copy)

- Patchsets must be iterative and fully bisectable, with clear comments
supplied in the commit message

- Any patchset with > 1 patch MUST have a cover letter

- All patches have a "Signed-off-by" indicating that the developer has
accepted the terms of the project license

- Each subject line should be in the format "[PATCH] subsystem: one line
comment" to enabled people to determine its relevance

2) Other developers start reviewing patches

There are several mini-workflows that tend to occur here so I'll try and
give some popular examples.

- If a patch is a build fix, one of the core committers will verify and
apply directly to the master repository

- If a maintainer is happy with the whole patchset, they reply to the
cover letter with a "Reviewed-by" and a line stating which subtree the
patchset has been applied to. Maintainers add a "Signed-off-by" to all
patches accepted via their subtree.

- A maintainer may indicate that the patch itself is fine, but the
commit message is not clear/incorrect and should be changed before being
resubmitted

- Any member of the mailing list may reply/review an individual patch by
hitting "reply" in their mail client. Patch comments are included
inline. If a reviewer is happy with an individual patch, they reply to
the patch and add a "Reviewed-by" line; anyone can provide a review,
even if they are not a maintainer

- For complex patches, a maintainer may request that the patchset may be
split into further patchsets. In general patchsets of > 20-30 patches
tend to be frowned upon, and will often immediately result in a reply
saying "please break this down into smaller chunks"

- A maintainer may reply and say that while the patchset in its final
form needs more work, some clean-ups introduced by the patch are ready
to be applied, e.g. please resubmit patches 1-4, 7 and 8 as a separate
patchset which can then be applied directly to a subtree

3) Patchset tidy-up and submission

After the initial review, new versions of the patchset are generally
reposted to the list within a few days.

- The patch version is included in the header with the same subject
line, e.g. "[PATCHv2] Add new feature"

- The cover letter contains a mini-changelog giving the changes between
v1 and v2 e.g.

v2:
Fix spelling mistake pointed out by Peter
Change lock ordering requested by Andreas

- "Reviewed-by" tags for individual patches sent to the list are
appended to the commit message for that patch before resubmission

Once a maintainer has accepted a patch into their subtree, they send a
pull request to the core maintainers to apply their trees to the main
repository. I appreciate that currently merge commits aren't used in the
PostgreSQL git repository so a pull request effectively becomes "rebase
this patchset onto master and push".

So why does this help patch review? From my experience I can definitely
say the following:

1) Patch review is very cheap

All I need to do is hit reply in my mail client and include an inline
comment for it to get picked up by a maintainer and the author - no need
to start messing around with attachments. Especially useful if I'm on a
mobile connection out of the office and not sat at my main PC.

2) Developers only need to review parts of the patchset that are
relevant to them

As an example if someone were to post a single large patch to -hackers
to change the way the stats collector worked then I'm extremely unlikely
to download and start reviewing it as I don't have any particular
expertise in this area.

However if it's posted as part of a patchset with a subject of "[PATCH
4/12] index: change retrieval of stats for GiST indexes" then that
immediately tells me that this patch is something I can review, and even
better I can do this in isolation without having to verify the entire patch.

3) Patch status is easy to spot

Searching on the subject line and looking for "Reviewed-by" tags means
it is possible to determine the status of each patchset really easily.

4) Patch bisection becomes very easy

A particular use case from QEMU: a patchset was applied changing the way
memory accesses work which broke one of my test images.

Using git bisect, not only could I determine it was this patchset that
broke my image, it was the particular patch that changed the routine for
handling unaligned memory accesses across pages. And even better, I knew
exactly from the log message who to email about it because of the
"Reviewed-by" tags for that one patch included in the commit message.

Compare this to say, for example, huge patches such as RLS. With this
being applied as a single monolithic patch then if a bug is found then
locating exactly which change causes an issue can be much harder and
time-consuming.

5) Individual patch comments are held in the project history

Verbose commit messages for each patch of a patchset allow someone to
understand the reasoning/history for each change with a simple bisection
without having to start searching the archives. In general the level of
documentation held within the repository for a patchset split into
several patches is greater than if it were simply applied as one single
commit with a single message.

6) Smaller patches are applied more often

By breaking large patches down into smaller chunks, even if the main
feature itself isn't ready to be applied to the main repository then a
lot of the preceding patches laying down the groundwork can.

The benefits here are that people with large out-of-tree patches find
that they reduce in size over time, and also trees tend to be merged
more often (every couple of weeks) since as large features get
implemented as a series of smaller patches, maintainers are more
confident about merging them. And if a groundwork patch does cause
breakage, this gets picked up much earlier in the process and becomes
very easy to isolate.

Another benefit of merging smaller patchsets more often is that
developers of larger patches end up spending less time rebasing patches
which have rotted in the weeks and months between review and merge (I've
seen this happen a few times with patches in the commitfest).

I hope that this email gives a reasonable argument as to why I believe
that patches submitted in this manner will get more review and merged
more often. However it's only fair that I should also point out the
negative parts I've seen from this approach:

1) Mailing list volume increases

The day after freeze was over, I had nearly 300 emails appear in my
inbox(!). Fortunately with the patch subject lines, it's easy to delete
the one's that aren't personally relevant to me, but it does takes a few
mins extra each day to filter my mails.

2) Unresponsive maintainers

A couple of patches I've supplied to QEMU have been ignored for months
because the maintainer for the subsystem just disappeared. Not so
relevant for PostgreSQL, although there are already implicit owners for
various parts of the code, e.g replication/WAL, indexes etc. But I'll
leave discussion of this point to other people :)

ATB,

Mark.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> #2 is solved by my previous comments about giving the CFM/C the
> authority. -Core could do that, they are in charge of release.

I don't think authority is the solution. Or certainly not one that
would work with an open source project like ours. What *would*
work is to identify and fix the friction points that prevent
people from joining, make the work harder than it needs to be,
and makes people stop reviewing? I could quickly identify a handful
of things, primarily among them the awful link-to-the-archives
to gather up all the patches process. We have git, let's use it
as it was intended.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
End Point Corporation http://www.endpoint.com/
PGP Key: 0x14964AC8 201412141011
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAlSNqK8ACgkQvJuQZxSWSsiewwCffAxv8xSZEyLWFz/b2+PxXOXS
xB4An2ubr7ovELtFMKZOZCsFHQAyVca4
=S6ZQ
-----END PGP SIGNATURE-----

On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
> Compare this to say, for example, huge patches such as RLS.

I specifically objected to that being flattened into a single monster
patch when I saw that'd been done. If you look at my part in the work on
the row security patch, while I was ultimately unsuccessful in getting
the patch mergeable I spent quite a bit of time splitting it up into a
logical patch-series for sane review and development. I am quite annoyed
that it was simply flattened back into an unreviewable, hard-to-follow
blob and committed in that form.

It's not like development on a patch series is difficult. You commit
small fixes and changes, then you 'git rebase -i' and reorder them to
apply to the appropriate changesets. Or you can do a 'rebase -i' and in
'e'dit mode make amendments to individual commits. Or you can commit
'fixup!'s that get auto-squashed.

This is part of my grumbling about the use of git like it's still CVS.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:

> If I could name just one thing that I think would improve things it
> would be submission of patches to the list in git format-patch format.
> Why? Because it enables two things: 1) by definition patches are
> "small-chunked" into individually reviewable pieces and 2) subject lines
> allow list members to filter things that aren't relevant to them.

Personally I do just that. Even though the official docs say context
diff is preferred, I think most people now use context diff in practice,
from 'git diff'.

I'm surprised most people don't use git format-patch .

Note that we can't just "git am" a patch from git format-patch at the
moment, because policy is that the committer should be the Author field.
The project would have to define a transition point where we started
using the git Author field for the author and the separate Committer
field, like git-am does by default. I'm not sure that'll ever actually
happen, or that it's even desirable.

> - Patchsets must be iterative and fully bisectable, with clear comments
> supplied in the commit message

Fully support this one.

Also in the smallest reasonable size divisions.

> - If a maintainer is happy with the whole patchset, they reply to the
> cover letter with a "Reviewed-by" and a line stating which subtree the
> patchset has been applied to. Maintainers add a "Signed-off-by" to all
> patches accepted via their subtree.

Sounds a lot like the kernel workflow (though in practice it seems like
the kernel folks tend bypass all this mailing list guff and just use
direct pulls/pushes for lots of things).

> - Any member of the mailing list may reply/review an individual patch by
> hitting "reply" in their mail client. Patch comments are included
> inline. If a reviewer is happy with an individual patch, they reply to
> the patch and add a "Reviewed-by" line; anyone can provide a review,
> even if they are not a maintainer

This is "fun" with many mail clients that like to linewrap text bodies
and don't permit inline replies to attached text.

> 6) Smaller patches are applied more often
>
> By breaking large patches down into smaller chunks, even if the main
> feature itself isn't ready to be applied to the main repository then a
> lot of the preceding patches laying down the groundwork can.

I think PostgreSQL does OK on smaller patches - at least sometimes.
There can be a great deal of bikeshedding and endless arguments,
back-and-forth about utility, in-tree users, etc, but no formal process
is ever going to change that. The process of getting an uncontroversial
patch into Pg is generally painless, if often rather slow unless a
committer sees it and commits it immediately upon it being posted.

> The benefits here are that people with large out-of-tree patches

I'm not sure we have all that many people in that category - though I'm
a part of a team that most certainly does fit the bill.

Even the "small" patches for BDR have been challenging and often
contentious though.

> Since as large features get
> implemented as a series of smaller patches

A big barrier here is the "we must have in-tree users" thing, which
makes it hard to do iterative work on a finer grained scale.

Some C-level unit tests that let us exercise small units of
functionality might ease those complaints. Right now the closest we have
to that is contrib/test_[blah] which is only useful for public API and
even then quite limited.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 14/12/14 15:51, Craig Ringer wrote:

> On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
>> Compare this to say, for example, huge patches such as RLS.
>
> I specifically objected to that being flattened into a single monster
> patch when I saw that'd been done. If you look at my part in the work on
> the row security patch, while I was ultimately unsuccessful in getting
> the patch mergeable I spent quite a bit of time splitting it up into a
> logical patch-series for sane review and development. I am quite annoyed
> that it was simply flattened back into an unreviewable, hard-to-follow
> blob and committed in that form.
>
> It's not like development on a patch series is difficult. You commit
> small fixes and changes, then you 'git rebase -i' and reorder them to
> apply to the appropriate changesets. Or you can do a 'rebase -i' and in
> 'e'dit mode make amendments to individual commits. Or you can commit
> 'fixup!'s that get auto-squashed.
>
> This is part of my grumbling about the use of git like it's still CVS.

I just want to add that I'm always grateful for the time that yourself
and all committers put into PostgreSQL, and my example of RLS was really
to signify "big feature X" rather than to pick on a particular aspect of
that patch. I apologise if you though I was in any way criticising the
work that you, or anyone else, put into this feature.

The argument I wanted to make is that if someone starts seeing a problem
with a current build of PostgreSQL and git bisects it down to a
particular commit, then smaller, iterative commits are extremely helpful
in this regard.

When searching for a needle in a haystack, there is very big difference
between a "add big feature X" haystack and a "change struct alignment
for Y" haystack, the latter which is implicit in having smaller,
iterative patchsets.

ATB,

Mark.

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
>> Compare this to say, for example, huge patches such as RLS.

> I specifically objected to that being flattened into a single monster
> patch when I saw that'd been done. If you look at my part in the work on
> the row security patch, while I was ultimately unsuccessful in getting
> the patch mergeable I spent quite a bit of time splitting it up into a
> logical patch-series for sane review and development. I am quite annoyed
> that it was simply flattened back into an unreviewable, hard-to-follow
> blob and committed in that form.

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.

Also, speaking of the larger picture: the current Postgres revision
history amounts to 37578 commits (as of sometime yesterday) --- and that's
just in the HEAD branch. If we'd made an effort to break feature patches
into bite-size chunks like you're recommending here, we'd probably have
easily half a million commits in the mainline history. That would not be
convenient to work with, and I really doubt that it would be more useful
for "git bisect" purposes, and I'll bet a large amount of money that most
of them would not have had commit messages composed with any care at all.

regards, tom lane

On 14/12/14 15:57, Craig Ringer wrote:

> On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
>
>> If I could name just one thing that I think would improve things it
>> would be submission of patches to the list in git format-patch format.
>> Why? Because it enables two things: 1) by definition patches are
>> "small-chunked" into individually reviewable pieces and 2) subject lines
>> allow list members to filter things that aren't relevant to them.
>
> Personally I do just that. Even though the official docs say context
> diff is preferred, I think most people now use context diff in practice,
> from 'git diff'.
>
> I'm surprised most people don't use git format-patch .
>
> Note that we can't just "git am" a patch from git format-patch at the
> moment, because policy is that the committer should be the Author field.
> The project would have to define a transition point where we started
> using the git Author field for the author and the separate Committer
> field, like git-am does by default. I'm not sure that'll ever actually
> happen, or that it's even desirable.

Sure, I appreciate that. The part I particularly wanted to emphasise
here was the use of a system identifier as part of the subject line,
e.g. say if there was a hypothetical patchset which sped up PostgreSQL
by 20% then you could see subjects like this:

[PATCH 1/x] lmgr: reduce lock strength for LWLock
[PATCH 2/x] wal: change lock ordering for WAL writes
[PATCH 3/x] optimiser: exit early if lock unobtainable

While these are obviously unrealistic, what I hope here is that people
with interest in these areas would take note of individual patches even
if they aren't interested in the entire patchset.

So maybe since Andreas did some recent locking work, he spots "lmgr" in
the subject and then makes a note to review that part of the patch.
Similary Heikki could spot "wal" and make a note to look at that, whilst
Tom would zero in on the optimiser changes.

The aim here is that no one person needs to sit and initially review a
complete patch before returning feedback to the developer.

>> - Patchsets must be iterative and fully bisectable, with clear comments
>> supplied in the commit message
>
> Fully support this one.
>
> Also in the smallest reasonable size divisions.

I should add here that the QEMU folk do tend to go to great lengths to
preserve bisectability; often intermediate compatibility APIs are
introduced early in the patchset and then removed at the very end when
the final feature is implemented.

>> - If a maintainer is happy with the whole patchset, they reply to the
>> cover letter with a "Reviewed-by" and a line stating which subtree the
>> patchset has been applied to. Maintainers add a "Signed-off-by" to all
>> patches accepted via their subtree.
>
> Sounds a lot like the kernel workflow (though in practice it seems like
> the kernel folks tend bypass all this mailing list guff and just use
> direct pulls/pushes for lots of things).

Yes indeed - mainly from the people on the KVM module side. Again I'm
not saying that this workflow is correct for PostgreSQL, I was trying to
use this an example to explain *why* the workflow is done in this manner
and how it helps developers such as myself.

>> - Any member of the mailing list may reply/review an individual patch by
>> hitting "reply" in their mail client. Patch comments are included
>> inline. If a reviewer is happy with an individual patch, they reply to
>> the patch and add a "Reviewed-by" line; anyone can provide a review,
>> even if they are not a maintainer
>
> This is "fun" with many mail clients that like to linewrap text bodies
> and don't permit inline replies to attached text.

Wow really? I can't say I've seen this in practice for a long time but I
defer to your experience here.

>> 6) Smaller patches are applied more often
>>
>> By breaking large patches down into smaller chunks, even if the main
>> feature itself isn't ready to be applied to the main repository then a
>> lot of the preceding patches laying down the groundwork can.
>
> I think PostgreSQL does OK on smaller patches - at least sometimes.
> There can be a great deal of bikeshedding and endless arguments,
> back-and-forth about utility, in-tree users, etc, but no formal process
> is ever going to change that. The process of getting an uncontroversial
> patch into Pg is generally painless, if often rather slow unless a
> committer sees it and commits it immediately upon it being posted.

I agree that trivial patches do tend to get picked up reasonably
quickly. It strikes me that it may prevent patches slipping through the
list if someone were officially nominated as a trivial patch monitor,
i.e. someone for developers to "ping" if their patch has been ignored
but it sounds like this is not such an issue in practice.

>> The benefits here are that people with large out-of-tree patches
>
> I'm not sure we have all that many people in that category - though I'm
> a part of a team that most certainly does fit the bill.
>
> Even the "small" patches for BDR have been challenging and often
> contentious though.
>
>> Since as large features get
>> implemented as a series of smaller patches
>
> A big barrier here is the "we must have in-tree users" thing, which
> makes it hard to do iterative work on a finer grained scale.
>
> Some C-level unit tests that let us exercise small units of
> functionality might ease those complaints. Right now the closest we have
> to that is contrib/test_[blah] which is only useful for public API and
> even then quite limited.

Agreed. In the end my thoughts on how to increase I feel we can increase
patch review can be summarised as this:

- Reduce patches into small, manageable and bisectable patchsets

- Indicate in the subject line the subsystem to attract the attention of
people who have knowledge/interest in that area

- Encourage review of individual simpler patches from non-core
developers; committers can then focus on their specialist areas for the
remaining patches

- Actively aim to merge reviewed patches where possible, thus reducing
the size of outstanding patchsets requiring review (particularly for
committers) upon the next patch iteration

ATB,

Mark.

On 12/14/2014 12:05 PM, Tom Lane wrote:
> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
>> On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
>>> Compare this to say, for example, huge patches such as RLS.
>> I specifically objected to that being flattened into a single monster
>> patch when I saw that'd been done. If you look at my part in the work on
>> the row security patch, while I was ultimately unsuccessful in getting
>> the patch mergeable I spent quite a bit of time splitting it up into a
>> logical patch-series for sane review and development. I am quite annoyed
>> that it was simply flattened back into an unreviewable, hard-to-follow
>> blob and committed in that form.
> 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.
>
> Also, speaking of the larger picture: the current Postgres revision
> history amounts to 37578 commits (as of sometime yesterday) --- and that's
> just in the HEAD branch. If we'd made an effort to break feature patches
> into bite-size chunks like you're recommending here, we'd probably have
> easily half a million commits in the mainline history. That would not be
> convenient to work with, and I really doubt that it would be more useful
> for "git bisect" purposes, and I'll bet a large amount of money that most
> of them would not have had commit messages composed with any care at all.

I have tried to stay away from this thread, but ...

I'm also quite dubious about this suggested workflow, partly for the
reasons Tom gives, and partly because it would constrain the way I work.
I tend to commit with little notes to myself in the commit logs, notes
that are never intended to become part of the public project history. I
should be quite sad to lose that.

As for using git bisect, usually when I do this each iteration is quite
expensive. Multiplying the number of commits by a factor between 10 and
100, which is what I think this would involve, would just make git
bisect have to do between 3 and 7 more iterations, ISTM. That's not a win.

On the larger issue, let me just note that I don't believe we have what
is fundamentally a technological problem, and while technological
changes can of course sometimes make things easier, they can also blind
us to the more basic problems we are facing.

cheers

andrew

On 14/12/14 17:05, Tom Lane wrote:

> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
>> On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
>>> Compare this to say, for example, huge patches such as RLS.
>
>> I specifically objected to that being flattened into a single monster
>> patch when I saw that'd been done. If you look at my part in the work on
>> the row security patch, while I was ultimately unsuccessful in getting
>> the patch mergeable I spent quite a bit of time splitting it up into a
>> logical patch-series for sane review and development. I am quite annoyed
>> that it was simply flattened back into an unreviewable, hard-to-follow
>> blob and committed in that form.
>
> 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.

The way in which this is normally handled is via the cover letter which
is going to be nicely captured in the mail archives; typically a cover
letter for a patchset consists of several paragraphs along the lines of
patches 1-3 do some re-org work, patch 4 reworks the Y API for new
feature X implemented in patches 9-12.

As an example take a look at the cover letter for this patch submitted
over the past few days:
http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg01990.html. It
seems to me that this would give the level of detail which you are
looking for.

I agree that definitely in some cases patches could be broken into "too
fine" pieces, but then I think it's perfectly okay for committers to
turn around and request the series be re-submitted in more sensible
chunks if they feel things have gone too far the other way.

> Also, speaking of the larger picture: the current Postgres revision
> history amounts to 37578 commits (as of sometime yesterday) --- and that's
> just in the HEAD branch. If we'd made an effort to break feature patches
> into bite-size chunks like you're recommending here, we'd probably have
> easily half a million commits in the mainline history. That would not be
> convenient to work with, and I really doubt that it would be more useful
> for "git bisect" purposes, and I'll bet a large amount of money that most
> of them would not have had commit messages composed with any care at all.

Having worked on a few kernel patches myself, git is surprisingly
effective on huge repositories once the cache is warmed up so I don't
feel this would be an issue with a PostgreSQL git repository which will
be many magnitudes smaller.

In terms of commit messages, I don't know if you missed that part of my
original response to Craig but it's considered normal for a maintainer
to reject a patch because of an incorrect/irrelevant commit message.

So if I submitted a patch that fixed a particular bug but you as a
committer weren't happy with the commit message, then I'm perfectly okay
with you asking me to resubmit with updated/corrected patch message wording.

ATB,

Mark.

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.

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.
--
Peter Geoghegan

On 14/12/14 17:30, Andrew Dunstan wrote:

> On 12/14/2014 12:05 PM, Tom Lane wrote:
>> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
>>> On 12/14/2014 10:35 PM, Mark Cave-Ayland wrote:
>>>> Compare this to say, for example, huge patches such as RLS.
>>> I specifically objected to that being flattened into a single monster
>>> patch when I saw that'd been done. If you look at my part in the work on
>>> the row security patch, while I was ultimately unsuccessful in getting
>>> the patch mergeable I spent quite a bit of time splitting it up into a
>>> logical patch-series for sane review and development. I am quite annoyed
>>> that it was simply flattened back into an unreviewable, hard-to-follow
>>> blob and committed in that form.
>> 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.
>>
>> Also, speaking of the larger picture: the current Postgres revision
>> history amounts to 37578 commits (as of sometime yesterday) --- and
>> that's
>> just in the HEAD branch. If we'd made an effort to break feature patches
>> into bite-size chunks like you're recommending here, we'd probably have
>> easily half a million commits in the mainline history. That would not be
>> convenient to work with, and I really doubt that it would be more useful
>> for "git bisect" purposes, and I'll bet a large amount of money that most
>> of them would not have had commit messages composed with any care at all.
>
> I have tried to stay away from this thread, but ...
>
> I'm also quite dubious about this suggested workflow, partly for the
> reasons Tom gives, and partly because it would constrain the way I work.
> I tend to commit with little notes to myself in the commit logs, notes
> that are never intended to become part of the public project history. I
> should be quite sad to lose that.

Interestingly enough, I tend to work in a very similar way to this. When
submitting patches upstream, I tend to rebase on a new branch and then
squash/rework as required.

One thing I do tend to find is that once I start rebasing the patches
for upstream, I find that many of my personal notes can be rewritten as
part of the patch comment (I would like to think that comments useful to
myself will be useful to other developers some day). Once the rebase is
complete, often I find that I have no non-public comments left so that
problem becomes almost non-existent.

> As for using git bisect, usually when I do this each iteration is quite
> expensive. Multiplying the number of commits by a factor between 10 and
> 100, which is what I think this would involve, would just make git
> bisect have to do between 3 and 7 more iterations, ISTM. That's not a win.

I find that this isn't too bad in practice. If you think about
monolithic patches during a commitfest, you can imagine that most of
them will touch one of the core .h files which will require most things
to be rebuilt once applied during bisection.

With smaller iterative patchsets, each patch tends to only touch a
handful of files and so "make install" generally only needs to rebuild
and link a very small number of files which is reasonably quick. Since
all of the changes for global header files are contained in 1-2 patches
then the number of complete rebuilds isn't as many as you might expect.

> On the larger issue, let me just note that I don't believe we have what
> is fundamentally a technological problem, and while technological
> changes can of course sometimes make things easier, they can also blind
> us to the more basic problems we are facing.

Absolutely. I firmly believe that the right tools for the job are
available, it's really trying to find a workflow solution that works
well for everyone involved in the project.

ATB,

Mark.

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.

On 12/15/2014 02:46 AM, Mark Cave-Ayland wrote:
> Interestingly enough, I tend to work in a very similar way to this. When
> submitting patches upstream, I tend to rebase on a new branch and then
> squash/rework as required.

Same here, and I find it works really well ... when I do it properly.

I usually have a private development branch that's full of "fixup! "
commits, WIPs, awful commit messages, etc.

Then I have the tree that's been rebased, squashed, and tided up.

I periodically tidy my latest work and replace the old clean tree with
it, then start my new development branch on top of the new clean tree.

This gives me a somewhat nice looking, well ordered patch series to work
on top of, while retaining the flexibility to do lots of quick fixes etc.

Admittedly, sometimes the development tree gets a bit large and it takes
a while before I give it a proper cleanup. My current project being very
much in that category. Still - it works well in general.

> I find that this isn't too bad in practice. If you think about
> monolithic patches during a commitfest, you can imagine that most of
> them will touch one of the core .h files which will require most things
> to be rebuilt once applied during bisection.

It's not build time, it's test-run time. Especially if you can't
automate the test, or it isn't practical to.

That's a legitimate concern - but I don't think we'd see a blowout of
patch counts to quite the same degree.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 14/12/14 20:07, Craig Ringer wrote:

> On 12/15/2014 02:46 AM, Mark Cave-Ayland wrote:
>> Interestingly enough, I tend to work in a very similar way to this. When
>> submitting patches upstream, I tend to rebase on a new branch and then
>> squash/rework as required.
>
> Same here, and I find it works really well ... when I do it properly.
>
> I usually have a private development branch that's full of "fixup! "
> commits, WIPs, awful commit messages, etc.
>
> Then I have the tree that's been rebased, squashed, and tided up.
>
> I periodically tidy my latest work and replace the old clean tree with
> it, then start my new development branch on top of the new clean tree.
>
> This gives me a somewhat nice looking, well ordered patch series to work
> on top of, while retaining the flexibility to do lots of quick fixes etc.
>
> Admittedly, sometimes the development tree gets a bit large and it takes
> a while before I give it a proper cleanup. My current project being very
> much in that category. Still - it works well in general.

That describes my workflow fairly well too.

>> I find that this isn't too bad in practice. If you think about
>> monolithic patches during a commitfest, you can imagine that most of
>> them will touch one of the core .h files which will require most things
>> to be rebuilt once applied during bisection.
>
> It's not build time, it's test-run time. Especially if you can't
> automate the test, or it isn't practical to.
>
> That's a legitimate concern - but I don't think we'd see a blowout of
> patch counts to quite the same degree.

At the end of the day, each project finds its own level as to how
complex individual patches should be and what should comprise a
patchset. Over the past couple of years the QEMU process has evolved
into its current form as maintainers and developers have figured out
what works best for them, and I don't see that PostgreSQL would be any
different in this respect - it takes time to adapt to a new workflow and
get it right.

Having worked on various parts for PostGIS for around 10 years, I've had
my head stuck into various parts of the GiST code and have got to know a
few parts of it really well.

What I find frustrating is that I've come back from a workflow where
I've been reviewing/testing patches within months of joining a project
because the barrier for entry has been so low, and yet even with much
longer experience of the PostgreSQL codebase I feel I can't do the same
for patches submitted to the commitfest.

And why is this? It's because while I know very specific areas of the
code well, many patches span different areas of the source tree of which
I have little and/or no experience, which when supplied as a single
monolithic patch makes it impossible for me to give meaningful review to
all but a tiny proportion of them.

I believe that this is one of the main reasons why people struggle to
find patch reviewers, with the consequence being that the majority of
work falls back onto the CF manager and the committers which is why we
have the current situation.

ATB,

Mark.

On Sun, Dec 14, 2014 at 05:21:06PM +0000, Mark Cave-Ayland wrote:
> I should add here that the QEMU folk do tend to go to great lengths to
> preserve bisectability; often intermediate compatibility APIs are
> introduced early in the patchset and then removed at the very end when
> the final feature is implemented.

I agree with Tom on this, and I want to point out that certain software
projects benefit more from modularized development than others , e.g.
QEMU is an interface library and therefore probably does things in a
more modular way than usual. For example, they are probably not adding
new SQL commands or configuration settings in the same way Postgres
does. It would be interesting to compare the directory span of a
typical Postgres patch vs. a QEMU or Linux kernel one.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 2014-12-15 11:21:03 -0500, Bruce Momjian wrote:
> On Sun, Dec 14, 2014 at 05:21:06PM +0000, Mark Cave-Ayland wrote:
> > I should add here that the QEMU folk do tend to go to great lengths to
> > preserve bisectability; often intermediate compatibility APIs are
> > introduced early in the patchset and then removed at the very end when
> > the final feature is implemented.
>
> I agree with Tom on this, and I want to point out that certain software
> projects benefit more from modularized development than others , e.g.
> QEMU is an interface library and therefore probably does things in a
> more modular way than usual. For example, they are probably not adding
> new SQL commands or configuration settings in the same way Postgres
> does.

I'm not following. What do you mean with 'interface library'? I'm pretty
sure qemu very regularly adds features including configuration
settings/parameters.

> It would be interesting to compare the directory span of a
> typical Postgres patch vs. a QEMU or Linux kernel one.

I don't believe this really is a question of the type of project. I
think it's more that especially the kernel has had to deal with similar
problems at a much larger scale. And the granular approach somewhat
works for them.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 15/12/14 16:28, Andres Freund wrote:

> I don't believe this really is a question of the type of project. I
> think it's more that especially the kernel has had to deal with similar
> problems at a much larger scale. And the granular approach somewhat
> works for them.

Correct. My argument was that dividing patches into smaller, more
reviewable chunks lessens the barrier for reviewers since many people
can review the individual patches as appropriate to their area of
expertise.

The benefits of this are that the many parts of the patchset get
reviewed early by a number of people, which reduces the workload on the
core developers as they only need to focus on a small number of
individual patches. Hence patches get reworked and merged much more
quickly in this way.

This is in contrast to the commitfest system where a single patch is i)
often held until the next commitfest (where bitrot often sets in) and
ii) requires the reviewer to have good knowledge all of the areas
covered by the patch in order to give a meaningful review, which
significantly reduces the pool of available reviewers.

ATB,

Mark.

On Sat, Dec 13, 2014 at 1:37 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>
> On 12/12/2014 06:02 AM, Josh Berkus wrote:
> >
> > Speaking as the originator of commitfests, they were *always* intended
> > to be a temporary measure, a step on the way to something else like
> > continuous integration.
>
> I'd really like to see the project revisit some of the underlying
> assumptions that're being made in this discussion:
>
> - Patches must be email attachments to a mailing list
>
> - Changes must be committed by applying a diff
>
> ... and take a look at some of the options a git-based workflow might
> offer, especially in combination with some of the tools out there that
> help track working branches, run CI, etc.
>
> Having grown used to push/pull workflows with CI integration I find the
> PostgreSQL patch workflow very frustrating, especially for larger
> patches. It's particularly annoying to see a patch series squashed into
> a monster patch whenever it changes hands or gets rebased, because it's
> being handed around as a great honking diff not a git working branch.
>
> Is it time to stop using git like CVS?
>

Perhaps it is just my inexperience with it, but I find the way that
mediawiki, for example, uses git to be utterly baffling. The git log is
bloated with the same change being listed multiple times, at least once as
a commit and again as a merge. If your suggestion would be to go in that
direction, I don't think that would be an improvement.

Cheers,

Jeff

On Sun, Dec 14, 2014 at 2:24 PM, Mark Cave-Ayland
<mark(dot)cave-ayland(at)ilande(dot)co(dot)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.

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Dec 14, 2014 at 2:24 PM, Mark Cave-Ayland
> <mark(dot)cave-ayland(at)ilande(dot)co(dot)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.

Yeah; that would totally destroy the use of git bisect, which was supposed
to be an advantage of smaller patches.

Perhaps you could always design the patch split-up in such a way that
each incremental step still compiles, but that would greatly limit how
you could factorize the patch, so I'm not sure it comes out as a win.
If we're going to submit patches in small chunks I'd rather the chunks
are built around separation of concerns, and not held to "would this
compile in isolation?".

IOW, submission in chunks is one thing but it shouldn't necessarily
translate to committing in the same chunks.

regards, tom lane

On 12/15/2014 02:08 PM, Robert Haas wrote:
> On Sun, Dec 14, 2014 at 2:24 PM, Mark Cave-Ayland
> <mark(dot)cave-ayland(at)ilande(dot)co(dot)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.
>
> 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.
>

+1

I have in the past found the "bunch of patches" approach to be more that
somewhat troublesome, especially when one gets "here is an update to
patch nn of the series" and one has to try to compose a coherent set of
patches in order to test them. A case in point I recall was the original
Foreign Data Wrapper patchset.

cheers

andrew

On Sun, Dec 14, 2014 at 4:53 PM, Mark Cave-Ayland
<mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> wrote:
> What I find frustrating is that I've come back from a workflow where
> I've been reviewing/testing patches within months of joining a project
> because the barrier for entry has been so low, and yet even with much
> longer experience of the PostgreSQL codebase I feel I can't do the same
> for patches submitted to the commitfest.
>
> And why is this? It's because while I know very specific areas of the
> code well, many patches span different areas of the source tree of which
> I have little and/or no experience, which when supplied as a single
> monolithic patch makes it impossible for me to give meaningful review to
> all but a tiny proportion of them.

So, there are certainly some large patches that do that, and they
typically require a very senior reviewer. But there are lots of small
patches too, touching little enough that you can learn enough to give
them a decent review even if you've never looked at that before. I
find myself in that situation as a reviewer and committer pretty
regularly; being a committer doesn't magically make you an expert on
the entire code base. You can (and I do) focus your effort on the
things you know best, but you have to step outside your comfort zone
sometimes, or you never learn anything new.

I feel like we used to be better at encouraging people to participate
in the CF even if they were not experts, and to do the best they can
based on what they did know. That was a helpful dynamic. Sure, the
reviews weren't perfect, but more people helped, and reviewing some of
the patch well and some of it in a more cursory manner is way better
than reviewing none of it at all.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/15/2014 11:27 AM, Robert Haas wrote:
> I feel like we used to be better at encouraging people to participate
> in the CF even if they were not experts, and to do the best they can
> based on what they did know. That was a helpful dynamic. Sure, the
> reviews weren't perfect, but more people helped, and reviewing some of
> the patch well and some of it in a more cursory manner is way better
> than reviewing none of it at all.

Well, it was strongly expressed to me by a number of senior contributors
on this list and at the developer meeting that inexpert reviews were not
really wanted, needed or helpful. As such, I stopped recruiting new
reviewers (and, for that matter, doing them myself). I don't know if
the same goes for anyone else.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, Dec 15, 2014 at 11:52 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 12/15/2014 11:27 AM, Robert Haas wrote:
>> I feel like we used to be better at encouraging people to participate
>> in the CF even if they were not experts, and to do the best they can
>> based on what they did know. That was a helpful dynamic. Sure, the
>> reviews weren't perfect, but more people helped, and reviewing some of
>> the patch well and some of it in a more cursory manner is way better
>> than reviewing none of it at all.
>
> Well, it was strongly expressed to me by a number of senior contributors
> on this list and at the developer meeting that inexpert reviews were not
> really wanted, needed or helpful. As such, I stopped recruiting new
> reviewers (and, for that matter, doing them myself). I don't know if
> the same goes for anyone else.

Really? I thought we were pretty consistent in encouraging new reviewers.

--
Peter Geoghegan

On 2014-12-15 11:52:35 -0800, Josh Berkus wrote:
> On 12/15/2014 11:27 AM, Robert Haas wrote:
> > I feel like we used to be better at encouraging people to participate
> > in the CF even if they were not experts, and to do the best they can
> > based on what they did know. That was a helpful dynamic. Sure, the
> > reviews weren't perfect, but more people helped, and reviewing some of
> > the patch well and some of it in a more cursory manner is way better
> > than reviewing none of it at all.
>
> Well, it was strongly expressed to me by a number of senior contributors
> on this list and at the developer meeting that inexpert reviews were not
> really wanted, needed or helpful.

I think that's pretty far away from what was said.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/15/2014 03:16 PM, Andres Freund wrote:
> On 2014-12-15 11:52:35 -0800, Josh Berkus wrote:
>> On 12/15/2014 11:27 AM, Robert Haas wrote:
>>> I feel like we used to be better at encouraging people to participate
>>> in the CF even if they were not experts, and to do the best they can
>>> based on what they did know. That was a helpful dynamic. Sure, the
>>> reviews weren't perfect, but more people helped, and reviewing some of
>>> the patch well and some of it in a more cursory manner is way better
>>> than reviewing none of it at all.
>> Well, it was strongly expressed to me by a number of senior contributors
>> on this list and at the developer meeting that inexpert reviews were not
>> really wanted, needed or helpful.
> I think that's pretty far away from what was said.
>

I welcome reviews at all levels, both as a developer and as a committer.

It is true that we are very short on reviewers with in depth knowledge
and experience, and this is the real problem we have far more than any
technological issues people might have.

But that doesn't mean we should be turning anyone away. We should not.

cheers

andrew

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-12-15 11:52:35 -0800, Josh Berkus wrote:
>> On 12/15/2014 11:27 AM, Robert Haas wrote:
>>> I feel like we used to be better at encouraging people to participate
>>> in the CF even if they were not experts, and to do the best they can
>>> based on what they did know. That was a helpful dynamic. Sure, the
>>> reviews weren't perfect, but more people helped, and reviewing some of
>>> the patch well and some of it in a more cursory manner is way better
>>> than reviewing none of it at all.

>> Well, it was strongly expressed to me by a number of senior contributors
>> on this list and at the developer meeting that inexpert reviews were not
>> really wanted, needed or helpful.

> I think that's pretty far away from what was said.

Doesn't match my recollection either.

regards, tom lane

On 12/15/2014 12:05 PM, Peter Geoghegan wrote:
> On Mon, Dec 15, 2014 at 11:52 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> On 12/15/2014 11:27 AM, Robert Haas wrote:
>>> I feel like we used to be better at encouraging people to participate
>>> in the CF even if they were not experts, and to do the best they can
>>> based on what they did know. That was a helpful dynamic. Sure, the
>>> reviews weren't perfect, but more people helped, and reviewing some of
>>> the patch well and some of it in a more cursory manner is way better
>>> than reviewing none of it at all.
>>
>> Well, it was strongly expressed to me by a number of senior contributors
>> on this list and at the developer meeting that inexpert reviews were not
>> really wanted, needed or helpful. As such, I stopped recruiting new
>> reviewers (and, for that matter, doing them myself). I don't know if
>> the same goes for anyone else.
>
> Really? I thought we were pretty consistent in encouraging new reviewers.
>

Read the thread on this list where I suggested crediting reviewers in
the release notes.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2014-12-15 16:14:30 -0800, Josh Berkus wrote:
> Read the thread on this list where I suggested crediting reviewers in
> the release notes.

Man. You're equating stuff that's not the same. You didn't get your way
(and I'm tentatively on your side onthat one) and take that to imply
that we don't want more reviewers.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Dec 15, 2014 at 03:29:19PM -0500, Andrew Dunstan wrote:
> On 12/15/2014 03:16 PM, Andres Freund wrote:
> >On 2014-12-15 11:52:35 -0800, Josh Berkus wrote:
> >>On 12/15/2014 11:27 AM, Robert Haas wrote:
> >>>I feel like we used to be better at encouraging people to participate
> >>>in the CF even if they were not experts, and to do the best they can
> >>>based on what they did know. That was a helpful dynamic. Sure, the
> >>>reviews weren't perfect, but more people helped, and reviewing some of
> >>>the patch well and some of it in a more cursory manner is way better
> >>>than reviewing none of it at all.
> >>Well, it was strongly expressed to me by a number of senior contributors
> >>on this list and at the developer meeting that inexpert reviews were not
> >>really wanted, needed or helpful.
> >I think that's pretty far away from what was said.
> >
>
>
> I welcome reviews at all levels, both as a developer and as a committer.
>
> It is true that we are very short on reviewers with in depth knowledge and
> experience, and this is the real problem we have far more than any
> technological issues people might have.
>
> But that doesn't mean we should be turning anyone away. We should not.

+1. Some of the best reviews I've seen are ones where the reviewer expressed
doubts about the review's quality, so don't let such doubts keep you from
participating. Every defect you catch saves a committer time; a review that
finds 3 of the 10 defects in a patch is still a big help. Some patch
submissions waste the community's time, but it's almost impossible to waste
the community's time by posting a patch review.

Confusion may have arisen due to statements that we need more expert
reviewers, which is also true. (When an expert writes a sufficiently-complex
patch, it's important that a second expert examine the patch at some point.)
If you're a novice reviewer, your reviews do help to solve that problem by
reducing the workload on expert reviewers.

Thanks,
nm

On 12/15/2014 07:34 PM, Andres Freund wrote:
> On 2014-12-15 16:14:30 -0800, Josh Berkus wrote:
>> Read the thread on this list where I suggested crediting reviewers in
>> the release notes.
>
> Man. You're equating stuff that's not the same. You didn't get your way
> (and I'm tentatively on your side onthat one) and take that to imply
> that we don't want more reviewers.

During that thread a couple people said that novice reviewers added no
value to the review process, and nobody argued with them then. I've
also been told this to my face at pgCon, and when I've tried organizing
patch review events. I got the message, which is why I stopped trying
to get new reviewers.

And frankly: if we're opposed to giving credit to patch reviewers, we're
opposed to having them.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2014-12-15 21:18:40 -0800, Josh Berkus wrote:
> On 12/15/2014 07:34 PM, Andres Freund wrote:
> > On 2014-12-15 16:14:30 -0800, Josh Berkus wrote:
> >> Read the thread on this list where I suggested crediting reviewers in
> >> the release notes.
> >
> > Man. You're equating stuff that's not the same. You didn't get your way
> > (and I'm tentatively on your side onthat one) and take that to imply
> > that we don't want more reviewers.
>
> During that thread a couple people said that novice reviewers added no
> value to the review process, and nobody argued with them then. I've
> also been told this to my face at pgCon, and when I've tried organizing
> patch review events. I got the message, which is why I stopped trying
> to get new reviewers.

I think there's a very large difference in what novice reviewers do. A
schematic 'in context format, compiles and survives make check' type of
test indeed doesn't seem to be particularly useful to me. A novice
reviewer that tries out the feature by reading the docs noticing
shortages there on the way, and then verifies that the feature works
outside of the two regression tests added is something entirely
different. Novice reviewers *can* review the code quality as well - it's
just that many we had didn't.

I think the big problem is that a good review takes time - and that's
what many of the novice reviewers I've observed weren't really aware of.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 16 December 2014 at 18:18, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
> > Man. You're equating stuff that's not the same. You didn't get your way
> > (and I'm tentatively on your side onthat one) and take that to imply
> > that we don't want more reviewers.
>
> During that thread a couple people said that novice reviewers added no
> value to the review process, and nobody argued with them then. I've
> also been told this to my face at pgCon, and when I've tried organizing
> patch review events. I got the message, which is why I stopped trying
> to get new reviewers.
>
> And frankly: if we're opposed to giving credit to patch reviewers, we're
> opposed to having them.
>
>
>
I'd just like to add something which might be flying below the radar of
more senior people. There are people out there (ike me) working on
PostgreSQL more for the challenge and perhaps the love of the product, who
make absolutely zero money out of it. For these people getting credit where
it's due is very important. I'm pretty happy with this at the moment and I
can't imagine any situation where not crediting reviewers would be
beneficial to anyone.

Regards

David Rowley

On 12/16/2014 03:08 AM, Robert Haas wrote:
> On Sun, Dec 14, 2014 at 2:24 PM, Mark Cave-Ayland
> <mark(dot)cave-ayland(at)ilande(dot)co(dot)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.

Absolutely. I think that'd be awful.

I'm a fan of fairly fine grained patch series, but only where each patch
makes some sense by its self and doesn't break the tree. Splitting stuff
up purely for the sake of it or having patches that are non-working
intermediate states is painful and pointless.

Occasoinally it can be worth having a patch that introduces intermediate
compatibility code that's removed later, especially when it's small or
very self-contained. Most of the time I'm inclined to think that's not
worth it and it can create annoying noise in the history.

I'm only advocating it where the individual parts make a reasonable
amount of sense standing alone, and actually work by themselves.

Anyway, I'm not contributing much to the real topic here, which is
commitfest process issues. It's probably getting toward time for me to
butt out.

> 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.

Yup ... and with 'git am' and then 'git diff' on subtrees, it's pretty
convenient.

It's even easier when someone pushes work to GitHub working trees, so
you don't have to mess about with applying the changes, but that's a
trivial and unimportant convenience.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

> On Sun, Dec 14, 2014 at 2:24 PM, Mark Cave-Ayland
> <mark(dot)cave-ayland(at)ilande(dot)co(dot)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.

On 15/12/14 19:24, Andrew Dunstan wrote:

> On 12/15/2014 02:08 PM, Robert Haas wrote:
>> On Sun, Dec 14, 2014 at 2:24 PM, Mark Cave-Ayland
>> <mark(dot)cave-ayland(at)ilande(dot)co(dot)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.
>>
>> 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.
>>
>
> +1
>
> I have in the past found the "bunch of patches" approach to be more that
> somewhat troublesome, especially when one gets "here is an update to
> patch nn of the series" and one has to try to compose a coherent set of
> patches in order to test them. A case in point I recall was the original
> Foreign Data Wrapper patchset.

In practice, people don't tend to post updates to individual patches in
that way. What happens is that after a week or so of reviews, people go
away and rework the patch and come back with a complete updated patchset
clearly marked as [PATCHv2] with the same subject line and an updated
cover letter describing the changes, so a complete coherent patchset is
always available.

Now as I mentioned previously, one of the disadvantages of splitting
patches in this way is that mailing list volume tends to grow quite
quickly - hence the use of [PATCH] filters and system identifiers in the
subject line to help mitigate this.

Whilst the volume of mail was a shock to begin with, having stuck with
it for a while I personally find that the benefits to development
outweigh the costs. Each individual project is different, but I believe
that there are good ideas here that can be used to improve workflow,
particularly when it comes to patch review.

ATB,

Mark.

On 12/16/2014 05:53 PM, Mark Cave-Ayland wrote:
> In practice, people don't tend to post updates to individual patches in
> that way.

Exactly. Much like if you push a new revision of a working branch, you
repost all the changesets - or should.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

> On Sun, Dec 14, 2014 at 4:53 PM, Mark Cave-Ayland
> <mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> wrote:
>> What I find frustrating is that I've come back from a workflow where
>> I've been reviewing/testing patches within months of joining a project
>> because the barrier for entry has been so low, and yet even with much
>> longer experience of the PostgreSQL codebase I feel I can't do the same
>> for patches submitted to the commitfest.
>>
>> And why is this? It's because while I know very specific areas of the
>> code well, many patches span different areas of the source tree of which
>> I have little and/or no experience, which when supplied as a single
>> monolithic patch makes it impossible for me to give meaningful review to
>> all but a tiny proportion of them.
>
> So, there are certainly some large patches that do that, and they
> typically require a very senior reviewer. But there are lots of small
> patches too, touching little enough that you can learn enough to give
> them a decent review even if you've never looked at that before. I
> find myself in that situation as a reviewer and committer pretty
> regularly; being a committer doesn't magically make you an expert on
> the entire code base. You can (and I do) focus your effort on the
> things you know best, but you have to step outside your comfort zone
> sometimes, or you never learn anything new.

Right. Which is why I'm advocating the approach of splitting patches in
relevant chunks so that experts in those areas can review them in
parallel. And even better, if I then want to start digging into other
parts of the system as time and interest allow then I can choose to
begin by picking a subject line from a patchset and going through this
small individual patch in detail rather than a single large patch.

It has often been suggested that people learn best when studying a mix
of 80% of things they already know compared to 20% of things they don't.
At least with more granular patches people can find a comfortable ratio
of new/old material vs. a single large patch which could be 70-80% of
completely new material and therefore much more difficult to pick-up.

Another thought to ponder here is that by reviewing smaller patches
which takes less time, for the same time cost a reviewer with experience
in one specific area can in theory review and provide feedback on
another 2-3 patchsets which should help relieve patch review starvation
across patchset submissions.

ATB,

Mark.

On 15 December 2014 at 19:52, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 12/15/2014 11:27 AM, Robert Haas wrote:
>> I feel like we used to be better at encouraging people to participate
>> in the CF even if they were not experts, and to do the best they can
>> based on what they did know. That was a helpful dynamic. Sure, the
>> reviews weren't perfect, but more people helped, and reviewing some of
>> the patch well and some of it in a more cursory manner is way better
>> than reviewing none of it at all.
>
> Well, it was strongly expressed to me by a number of senior contributors
> on this list and at the developer meeting that inexpert reviews were not
> really wanted, needed or helpful. As such, I stopped recruiting new
> reviewers (and, for that matter, doing them myself). I don't know if
> the same goes for anyone else.

I don't remember saying that, hearing it or agreeing with it and I
don't agree with it now.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 16/12/14 04:57, Noah Misch wrote:

>> But that doesn't mean we should be turning anyone away. We should not.
>
> +1. Some of the best reviews I've seen are ones where the reviewer expressed
> doubts about the review's quality, so don't let such doubts keep you from
> participating. Every defect you catch saves a committer time; a review that
> finds 3 of the 10 defects in a patch is still a big help. Some patch
> submissions waste the community's time, but it's almost impossible to waste
> the community's time by posting a patch review.
>
> Confusion may have arisen due to statements that we need more expert
> reviewers, which is also true. (When an expert writes a sufficiently-complex
> patch, it's important that a second expert examine the patch at some point.)
> If you're a novice reviewer, your reviews do help to solve that problem by
> reducing the workload on expert reviewers.

I should add that by reducing the barrier of entry for patch review,
there is definitely potential to find common errors before a patch gets
analysed in detail by a committer.

For example I may not know a lot about certain PostgreSQL subsystems but
from a decent C coding background I can pick out certain suspicious
things in patches, e.g. potential overflows, pointer arithmetic bugs,
spelling mistakes/incorrect comments etc. and flag them to the original
submitter to double-check.

ATB,

Mark.

On 12/16/14 11:26 AM, Mark Cave-Ayland wrote:
> On 15/12/14 19:27, Robert Haas wrote:
>> So, there are certainly some large patches that do that, and they
>> typically require a very senior reviewer. But there are lots of small
>> patches too, touching little enough that you can learn enough to give
>> them a decent review even if you've never looked at that before. I
>> find myself in that situation as a reviewer and committer pretty
>> regularly; being a committer doesn't magically make you an expert on
>> the entire code base. You can (and I do) focus your effort on the
>> things you know best, but you have to step outside your comfort zone
>> sometimes, or you never learn anything new.
>
> Right. Which is why I'm advocating the approach of splitting patches in
> relevant chunks so that experts in those areas can review them in
> parallel.

I don't see how splitting patches up would help with that. I often look
at only the parts of patches that touch the things I've worked with
before. And in doing that, I've found that having the context there is
absolutely crucial almost every single time, since I commonly ask myself
"Why do we need to do this to implement feature X?", and only looking at
the rest of the complete patch (or patch set, however you want to think
about it) reveals that.

Of course, me looking at parts of patches, finding nothing questionable
and not sending an email about my findings (or lack thereof) hardly
counts as "review", so somebody else still has to review the actual
patch as a whole. Nor do I get any credit for doing any of that, which
might be a show-stopper for someone else. But I think that's just
because I'm not doing it correctly. I don't see why someone couldn't
comment on a patch saying "I've reviewed the grammar changes, and they
look good to me".

.marko

On 16/12/14 07:33, David Rowley wrote:

> On 16 December 2014 at 18:18, Josh Berkus <josh(at)agliodbs(dot)com
> <mailto:josh(at)agliodbs(dot)com>> wrote:
>
> > Man. You're equating stuff that's not the same. You didn't get your way
> > (and I'm tentatively on your side onthat one) and take that to imply
> > that we don't want more reviewers.
>
> During that thread a couple people said that novice reviewers added no
> value to the review process, and nobody argued with them then. I've
> also been told this to my face at pgCon, and when I've tried organizing
> patch review events. I got the message, which is why I stopped trying
> to get new reviewers.
>
> And frankly: if we're opposed to giving credit to patch reviewers, we're
> opposed to having them.
>
>
>
> I'd just like to add something which might be flying below the radar of
> more senior people. There are people out there (ike me) working on
> PostgreSQL more for the challenge and perhaps the love of the product,
> who make absolutely zero money out of it. For these people getting
> credit where it's due is very important. I'm pretty happy with this at
> the moment and I can't imagine any situation where not crediting
> reviewers would be beneficial to anyone.

This is exactly where I am at the moment, having previously been more
involved with the development side of PostgreSQL during the past.

Personally having a credit as a patch reviewer isn't particularly
important to me, since mail archives are good enough these days that if
people do query my contributions towards projects then I can point them
towards any reasonable search engine.

The biggest constraint on my ability to contribute is *time*.

Imagine the situation as a reviewer that I am currently on the mailing
list for two well-known open source projects and I also have a day job
and a home life to contend with.

For the spare time that I have for review, one of these projects
requires me to download attachment(s), apply them to a git tree
(hopefully it still applies), run a complete "make check" regression
series, try and analyse a patch which will often reference parts to
which I have no understanding, and then write up a coherent email and
submit it to the mailing list. Realistically to do all this and provide
a review that is going to be of use to a committer is going to take a
minimum of 1-2 hours, and even then there's a good chance that I've
easily missed obvious bugs in the parts of the system I don't understand
well.

For the second project, I can skim through my inbox daily picking up
specific areas I work on/are interested in, hit reply to add a couple of
lines of inline comments to the patch and send feedback to the
author/list in just a few minutes.

The obvious question is, of course, which project gets the majority
share of my spare review time?

ATB,

Mark.

On 16/12/14 10:49, Marko Tiikkaja wrote:

> On 12/16/14 11:26 AM, Mark Cave-Ayland wrote:
>> On 15/12/14 19:27, Robert Haas wrote:
>>> So, there are certainly some large patches that do that, and they
>>> typically require a very senior reviewer. But there are lots of small
>>> patches too, touching little enough that you can learn enough to give
>>> them a decent review even if you've never looked at that before. I
>>> find myself in that situation as a reviewer and committer pretty
>>> regularly; being a committer doesn't magically make you an expert on
>>> the entire code base. You can (and I do) focus your effort on the
>>> things you know best, but you have to step outside your comfort zone
>>> sometimes, or you never learn anything new.
>>
>> Right. Which is why I'm advocating the approach of splitting patches in
>> relevant chunks so that experts in those areas can review them in
>> parallel.
>
> I don't see how splitting patches up would help with that. I often look
> at only the parts of patches that touch the things I've worked with
> before. And in doing that, I've found that having the context there is
> absolutely crucial almost every single time, since I commonly ask myself
> "Why do we need to do this to implement feature X?", and only looking at
> the rest of the complete patch (or patch set, however you want to think
> about it) reveals that.

I've already covered this earlier in the thread so I won't go into too
much detail, but the overall "flow" of the patchset is described by the
cover letter (please feel free to look at the example link I posted).

In terms of individual patches within a patchset then if the combination
of the cover letter and individual commit message don't give you enough
context then the developer needs to fix this; either the patchset has
been split at a nonsensical place or either one or other of the cover
letter and/or commit message need to be revised.

> Of course, me looking at parts of patches, finding nothing questionable
> and not sending an email about my findings (or lack thereof) hardly
> counts as "review", so somebody else still has to review the actual
> patch as a whole. Nor do I get any credit for doing any of that, which
> might be a show-stopper for someone else. But I think that's just
> because I'm not doing it correctly. I don't see why someone couldn't
> comment on a patch saying "I've reviewed the grammar changes, and they
> look good to me".

Sure, there is always scope for doing that which is what splitting
patches and constant review encourages. In terms of the current
commitfest system, the process for review is clearly documented and as
I've pointed out in my response to David, extremely heavyweight in
comparison.

ATB,

Mark.

On Tue, Dec 16, 2014 at 8:09 AM, Mark Cave-Ayland
<mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> wrote:
> For the spare time that I have for review, one of these projects
> requires me to download attachment(s), apply them to a git tree
> (hopefully it still applies), run a complete "make check" regression
> series, try and analyse a patch which will often reference parts to
> which I have no understanding, and then write up a coherent email and
> submit it to the mailing list. Realistically to do all this and provide
> a review that is going to be of use to a committer is going to take a
> minimum of 1-2 hours, and even then there's a good chance that I've
> easily missed obvious bugs in the parts of the system I don't understand
> well.
>
> For the second project, I can skim through my inbox daily picking up
> specific areas I work on/are interested in, hit reply to add a couple of
> lines of inline comments to the patch and send feedback to the
> author/list in just a few minutes.

Notice though that on the second project, the review is made "in the
air". You didn't build, nor ran tests, you're guessing how the code
performs rather than seeing it perform, so the review is "light"
compared to the first.

Some of the effort of the first review is pointless, but not all of
it. Running make check may be a good task for a CI tool, but if you
ignore the result of make check, your review is missing an important
bit of information to be weighted.

There's something to be learned from the open build service (
http://openbuildservice.org/ ), there, review requests contain in the
interface the results of the build and rpmlint (it's for rpms). It
makes the review easy yet informed.

On Tue, Dec 16, 2014 at 11:09:34AM +0000, Mark Cave-Ayland wrote:
> On 16/12/14 07:33, David Rowley wrote:
>
> > On 16 December 2014 at 18:18, Josh Berkus <josh(at)agliodbs(dot)com
> > <mailto:josh(at)agliodbs(dot)com>> wrote:
> >
> > > Man. You're equating stuff that's not the same. You didn't get your way
> > > (and I'm tentatively on your side onthat one) and take that to imply
> > > that we don't want more reviewers.
> >
> > During that thread a couple people said that novice reviewers added no
> > value to the review process, and nobody argued with them then. I've
> > also been told this to my face at pgCon, and when I've tried organizing
> > patch review events. I got the message, which is why I stopped trying
> > to get new reviewers.
> >
> > And frankly: if we're opposed to giving credit to patch reviewers, we're
> > opposed to having them.
> >
> >
> >
> > I'd just like to add something which might be flying below the radar of
> > more senior people. There are people out there (ike me) working on
> > PostgreSQL more for the challenge and perhaps the love of the product,
> > who make absolutely zero money out of it. For these people getting
> > credit where it's due is very important. I'm pretty happy with this at
> > the moment and I can't imagine any situation where not crediting
> > reviewers would be beneficial to anyone.
>
> This is exactly where I am at the moment, having previously been more
> involved with the development side of PostgreSQL during the past.
>
> Personally having a credit as a patch reviewer isn't particularly
> important to me, since mail archives are good enough these days that if
> people do query my contributions towards projects then I can point them
> towards any reasonable search engine.
>
> The biggest constraint on my ability to contribute is *time*.
>
> Imagine the situation as a reviewer that I am currently on the mailing
> list for two well-known open source projects and I also have a day job
> and a home life to contend with.
>
> For the spare time that I have for review, one of these projects
> requires me to download attachment(s), apply them to a git tree
> (hopefully it still applies), run a complete "make check" regression
> series, try and analyse a patch which will often reference parts to
> which I have no understanding, and then write up a coherent email and
> submit it to the mailing list. Realistically to do all this and provide
> a review that is going to be of use to a committer is going to take a
> minimum of 1-2 hours, and even then there's a good chance that I've
> easily missed obvious bugs in the parts of the system I don't understand
> well.
>
> For the second project, I can skim through my inbox daily picking up
> specific areas I work on/are interested in, hit reply to add a couple of
> lines of inline comments to the patch and send feedback to the
> author/list in just a few minutes.

With utmost respect, you've missed something really important in the
second that the first has, and frankly isn't terribly onerous: does an
actual system produce working code? In the PostgreSQL case, you can
stop as soon as you discover that the patch doesn't apply to master or
that ./configure doesn't work, or that the code doesn't compile:
elapsed time <= 5 minutes. Or you can keep moving until you have made
progress for the time you've allotted.

But the bigger issue, as others have pointed out, has never been a
technical one. It's motivating people whose time is already much in
demand to spend some of it on reviewing.

I wasn't discouraged by the preliminary patch review process or any
feedback I got. My absence lately has more to do with other demands
on my time.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 16/12/14 13:37, Claudio Freire wrote:

>> For the second project, I can skim through my inbox daily picking up
>> specific areas I work on/are interested in, hit reply to add a couple of
>> lines of inline comments to the patch and send feedback to the
>> author/list in just a few minutes.
>
> Notice though that on the second project, the review is made "in the
> air". You didn't build, nor ran tests, you're guessing how the code
> performs rather than seeing it perform, so the review is "light"
> compared to the first.

I think this doing developers in general a little injustice here. The
general rule for a submitting patch to *any* project is: i) does it
apply to the current source tree and ii) does it build and pass the
regression tests?

Maybe there is a greater incidence of this happening in PostgreSQL
compared to other projects? But in any case the project has every right
to refuse further review if these 2 simple criteria are not met. Also
with a submission from git, you can near 100% guarantee that the author
has actually built and run the code before submission. I have seen
occasions where a patch has been submitted, and a committer will send a
quick email along the lines of "The patch looks good, but I've just
applied a patch that will conflict with this, so please rebase and
resubmit".

You mention about performance, but again I've seen from this list that
good developers can generally pick up on a lot of potential regressions
by eye, e.g. lookup times go from O(N) to O(N^2) without having to
resort to building and testing the code. And by breaking into small
chunks people can focus their time on reviewing the areas they are good at.

Occasionally sometimes people do get a patch ready for commit but it
fails build/test on one particular platform. In that case, the committer
simply posts the build/test failure to the list and requests that the
patchset be fixed ready for re-test. This is a very rare occurrence though.

Also you mentioned about "light" review but I wouldn't call it that. I
see it more as with each iteration the magnifying glass used to look at
the code gets bigger and bigger.

A lot of initial comments for the second project are along the lines of
"this doesn't look right - won't this overflow on values >2G?" or
"you're assuming here that A occurs before B, whereas if you run with
-foo this likely won't work". And this is the area which I believe will
have the greatest benefit for PostgreSQL - making it easier to catch and
rework the obvious flaws in the patch in a timely manner before it gets
to the committer.

ATB,

Mark.

On 16/12/14 13:44, David Fetter wrote:

> On Tue, Dec 16, 2014 at 11:09:34AM +0000, Mark Cave-Ayland wrote:
>> On 16/12/14 07:33, David Rowley wrote:
>>
>>> On 16 December 2014 at 18:18, Josh Berkus <josh(at)agliodbs(dot)com
>>> <mailto:josh(at)agliodbs(dot)com>> wrote:
>>>
>>> > Man. You're equating stuff that's not the same. You didn't get your way
>>> > (and I'm tentatively on your side onthat one) and take that to imply
>>> > that we don't want more reviewers.
>>>
>>> During that thread a couple people said that novice reviewers added no
>>> value to the review process, and nobody argued with them then. I've
>>> also been told this to my face at pgCon, and when I've tried organizing
>>> patch review events. I got the message, which is why I stopped trying
>>> to get new reviewers.
>>>
>>> And frankly: if we're opposed to giving credit to patch reviewers, we're
>>> opposed to having them.
>>>
>>>
>>>
>>> I'd just like to add something which might be flying below the radar of
>>> more senior people. There are people out there (ike me) working on
>>> PostgreSQL more for the challenge and perhaps the love of the product,
>>> who make absolutely zero money out of it. For these people getting
>>> credit where it's due is very important. I'm pretty happy with this at
>>> the moment and I can't imagine any situation where not crediting
>>> reviewers would be beneficial to anyone.
>>
>> This is exactly where I am at the moment, having previously been more
>> involved with the development side of PostgreSQL during the past.
>>
>> Personally having a credit as a patch reviewer isn't particularly
>> important to me, since mail archives are good enough these days that if
>> people do query my contributions towards projects then I can point them
>> towards any reasonable search engine.
>>
>> The biggest constraint on my ability to contribute is *time*.
>>
>> Imagine the situation as a reviewer that I am currently on the mailing
>> list for two well-known open source projects and I also have a day job
>> and a home life to contend with.
>>
>> For the spare time that I have for review, one of these projects
>> requires me to download attachment(s), apply them to a git tree
>> (hopefully it still applies), run a complete "make check" regression
>> series, try and analyse a patch which will often reference parts to
>> which I have no understanding, and then write up a coherent email and
>> submit it to the mailing list. Realistically to do all this and provide
>> a review that is going to be of use to a committer is going to take a
>> minimum of 1-2 hours, and even then there's a good chance that I've
>> easily missed obvious bugs in the parts of the system I don't understand
>> well.
>>
>> For the second project, I can skim through my inbox daily picking up
>> specific areas I work on/are interested in, hit reply to add a couple of
>> lines of inline comments to the patch and send feedback to the
>> author/list in just a few minutes.
>
> With utmost respect, you've missed something really important in the
> second that the first has, and frankly isn't terribly onerous: does an
> actual system produce working code? In the PostgreSQL case, you can
> stop as soon as you discover that the patch doesn't apply to master or
> that ./configure doesn't work, or that the code doesn't compile:
> elapsed time <= 5 minutes. Or you can keep moving until you have made
> progress for the time you've allotted.

As per my previous email, it's generally quite rare for a developer to
post non-working code to a list (in some cases someone will send a quick
reply pointing that this needs to be rebased because it appears to
reference an old API). From what I see in PostgreSQL this mostly happens
because of bitrot between the time the patch was posted to the list and
the actual commitfest itself - so in one way the commitfest system
exacerbates this particular problem further.

> But the bigger issue, as others have pointed out, has never been a
> technical one. It's motivating people whose time is already much in
> demand to spend some of it on reviewing.
>
> I wasn't discouraged by the preliminary patch review process or any
> feedback I got. My absence lately has more to do with other demands
> on my time.

I am completely in agreement with you here. My approach is more along
the lines of that I know access to long periods of time to review
patches is often impractical, and so how can the review process be made
more time-efficient in order to allow you, me and other people in
similar time-limited environments to be able to participate more?

ATB,

Mark.

On Tue, Dec 16, 2014 at 11:47 AM, Mark Cave-Ayland
<mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> wrote:
> On 16/12/14 13:37, Claudio Freire wrote:
>
>>> For the second project, I can skim through my inbox daily picking up
>>> specific areas I work on/are interested in, hit reply to add a couple of
>>> lines of inline comments to the patch and send feedback to the
>>> author/list in just a few minutes.
>>
>> Notice though that on the second project, the review is made "in the
>> air". You didn't build, nor ran tests, you're guessing how the code
>> performs rather than seeing it perform, so the review is "light"
>> compared to the first.
>
> I think this doing developers in general a little injustice here. The
> general rule for a submitting patch to *any* project is: i) does it
> apply to the current source tree and ii) does it build and pass the
> regression tests?

That it's a policy doesn't guarantee that people submitting patches
will adhere. They could be failing to adhere even unknowingly (ie:
bitrot - there's quite some time going on from first submission to
first review).

> Maybe there is a greater incidence of this happening in PostgreSQL
> compared to other projects?

Perhaps, but it's because the reviewers take care to test things
before they hit the build farm.

That basic testing really is something for a CI tool, like the build
farm itself, not reviewers. But you cannot wait until after comitting
to let the build farm tell you something's broken: you need CI results
*during* review.

> But in any case the project has every right
> to refuse further review if these 2 simple criteria are not met.

Nobody said otherwise

> Also
> with a submission from git, you can near 100% guarantee that the author
> has actually built and run the code before submission.

I don't see how. Forks don't have travis ci unless you add it, or am I mistaken?

> You mention about performance, but again I've seen from this list that
> good developers can generally pick up on a lot of potential regressions
> by eye, e.g. lookup times go from O(N) to O(N^2) without having to
> resort to building and testing the code. And by breaking into small
> chunks people can focus their time on reviewing the areas they are good at.

I meant performance as in running, not really performance. Sorry for
the confusion.

I meant, you're looking at the code and guessing how it runs, but you
don't really know. It's easy to make assumptions that are false while
reviewing, quickly disproved by actually running tests.

A light review without ever building can fail to detect those issues.
A heavier review patching up and building manually is too much manual
work that could really be automated. The optimum is somewhere between
those two extremes.

> Occasionally sometimes people do get a patch ready for commit but it
> fails build/test on one particular platform.

Again, pre-review CI can take care of this.

> In that case, the committer
> simply posts the build/test failure to the list and requests that the
> patchset be fixed ready for re-test. This is a very rare occurrence though.

It shouldn't need to reach the repo, don't you think?

> Also you mentioned about "light" review but I wouldn't call it that.

I've made my fair share of light reviews (not for pg though) and can
recognize the description. It is a light review what you described.

Surely not all reviews with inline comments are light reviews, but
what you described was indeed a light review.

> A lot of initial comments for the second project are along the lines of
> "this doesn't look right - won't this overflow on values >2G?" or
> "you're assuming here that A occurs before B, whereas if you run with
> -foo this likely won't work". And this is the area which I believe will
> have the greatest benefit for PostgreSQL - making it easier to catch and
> rework the obvious flaws in the patch in a timely manner before it gets
> to the committer.

If you read carefully my reply, I'm not at all opposed to that. I'm
just pointing out, that easier reviews need not result in better
reviews.

Better, easier reviews are those where the trivial reviews are
automated, as is done in the project I linked.

Formatting, whether it still applies, and whether it builds and checks
pass, all those are automatable.

On 16/12/14 15:42, Claudio Freire wrote:

>> Also
>> with a submission from git, you can near 100% guarantee that the author
>> has actually built and run the code before submission.
>
> I don't see how. Forks don't have travis ci unless you add it, or am I mistaken?

Well as I mentioned in my last email, practically all developers will
rebase and run "make check" on their patched tree before submitting to
the list. Hopefully there aren't hordes of developers deliberating
creating and submitting broken patches ;)

>> You mention about performance, but again I've seen from this list that
>> good developers can generally pick up on a lot of potential regressions
>> by eye, e.g. lookup times go from O(N) to O(N^2) without having to
>> resort to building and testing the code. And by breaking into small
>> chunks people can focus their time on reviewing the areas they are good at.
>
> I meant performance as in running, not really performance. Sorry for
> the confusion.

Okay no worries.

> I meant, you're looking at the code and guessing how it runs, but you
> don't really know. It's easy to make assumptions that are false while
> reviewing, quickly disproved by actually running tests.
>
> A light review without ever building can fail to detect those issues.
> A heavier review patching up and building manually is too much manual
> work that could really be automated. The optimum is somewhere between
> those two extremes.

Correct. My analogy here was that people with varying abilities review
the patches at their experience level, and once low-hanging/obvious
design issues have been resolved then more experienced developers will
start to review the patch at a deeper level.

>> Occasionally sometimes people do get a patch ready for commit but it
>> fails build/test on one particular platform.
>
> Again, pre-review CI can take care of this.

Yes - see my next reply...

>> In that case, the committer
>> simply posts the build/test failure to the list and requests that the
>> patchset be fixed ready for re-test. This is a very rare occurrence though.
>
> It shouldn't need to reach the repo, don't you think?

When I say repo, I mean the local repo of the tree maintainer. Currently
the tree maintainer pulls each merge request into his local tree,
performs a buildfarm test and only pushes the merge upstream once this
has passed. I guess the PostgreSQL equivalent of this would be having a
"merge-pending" branch on the buildfarm rather than just a post-commit
build of git master (which we see from reports to the list periodically
fails in this way) and only pushing a set of patches when the buildfarm
comes back green.

>> Also you mentioned about "light" review but I wouldn't call it that.
>
> I've made my fair share of light reviews (not for pg though) and can
> recognize the description. It is a light review what you described.
>
> Surely not all reviews with inline comments are light reviews, but
> what you described was indeed a light review.
>
>> A lot of initial comments for the second project are along the lines of
>> "this doesn't look right - won't this overflow on values >2G?" or
>> "you're assuming here that A occurs before B, whereas if you run with
>> -foo this likely won't work". And this is the area which I believe will
>> have the greatest benefit for PostgreSQL - making it easier to catch and
>> rework the obvious flaws in the patch in a timely manner before it gets
>> to the committer.
>
> If you read carefully my reply, I'm not at all opposed to that. I'm
> just pointing out, that easier reviews need not result in better
> reviews.
>
> Better, easier reviews are those where the trivial reviews are
> automated, as is done in the project I linked.

Yes I can definitely agree with that. See below again:

> Formatting, whether it still applies, and whether it builds and checks
> pass, all those are automatable.

I should add that QEMU provides a branch of checkpatch.pl in the source
tree which submitters are requested to run on their patchset before
submission to the list. This catches patches that don't meet the project
style guidelines including casing, braces, trailing whitespace,
tab/space issues etc. and that's before the patch is even submitted to
the list.

ATB,

Mark.

On Tue, Dec 16, 2014 at 2:33 AM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> I'd just like to add something which might be flying below the radar of more
> senior people. There are people out there (ike me) working on PostgreSQL
> more for the challenge and perhaps the love of the product, who make
> absolutely zero money out of it. For these people getting credit where it's
> due is very important. I'm pretty happy with this at the moment and I can't
> imagine any situation where not crediting reviewers would be beneficial to
> anyone.

We routinely and regularly contribute reviews in the commit logs for
precisely this reason. I don't think anyone is opposed to that.
There is some opposition to crediting them in the release notes
because the one time Bruce tried it made for an enormous volume of
additional text in the release notes, and there were cases where
people's names were mentioned on relatively equal footing when their
contributions were very much unequal. For example, let's take a look
at the commit message for Hot Standby:

Simon Riggs, with significant and lengthy review by Heikki
Linnakangas, including streamlined redesign of snapshot creation and
two-phase commit.

Important contributions from Florian Pflug, Mark Kirkwood, Merlin
Moncure, Greg Stark, Gianni Ciolli, Gabriele Bartolini, Hannu Krosing,
Robert Haas, Tatsuo Ishii, Hiroyuki Yamada plus support and feedback
from many other community members.

The release note ended up looking like this:

Allow a standby server to accept read-only queries (Simon Riggs,
Heikki Linnakangas)

Including all of the other names of people who made important
contributions, many of which consisted of reviewing, would make that
release note item - and many others - really, really long, so I'm not
in favor of that. Crediting reviewers is important, but so is having
the release notes be readable.

It has been proposed that we do a general list of people at the bottom
of the release notes who helped review during that cycle. That would
be less intrusive and possibly a good idea, but would we credit the
people who did a TON of reviewing? Everyone who reviewed even one
patch? Somewhere in between? Would committers be excluded because "we
just expect them to help" or included because credit is important to
established community members too? To what extent would this be
duplicative of http://www.postgresql.org/community/contributors/ ?
I'm not necessarily averse to doing something here, but the reason why
nothing has happened has much more to do with the fact that it's hard
to figure out exactly what the best thing would be than any idea that
"we don't want to credit reviewers". We do want to credit reviewers,
AND WE DO, as a quick look at 'git log' will speedily reveal.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Dec 16, 2014 at 12:18 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 12/15/2014 07:34 PM, Andres Freund wrote:
>> On 2014-12-15 16:14:30 -0800, Josh Berkus wrote:
>>> Read the thread on this list where I suggested crediting reviewers in
>>> the release notes.
>>
>> Man. You're equating stuff that's not the same. You didn't get your way
>> (and I'm tentatively on your side onthat one) and take that to imply
>> that we don't want more reviewers.
>
> During that thread a couple people said that novice reviewers added no
> value to the review process, and nobody argued with them then. I've
> also been told this to my face at pgCon, and when I've tried organizing
> patch review events. I got the message, which is why I stopped trying
> to get new reviewers.

I think what was said is that it isn't very useful to have reviewers
who just report that the patch applies and passes make check. But any
review that does any study of the code, or finds a bug in the
functionality, or reports that the patch does NOT apply and/or pass
make check, or suggests a way that the documentation could be
improved, or fixes a typo in a comment, or diagnoses a whitespace
error is useful. Summarizing that as "novice reviewers added no value
to the review process" is like summarizing the plot of Superman as
"alien invades earth".

> And frankly: if we're opposed to giving credit to patch reviewers, we're
> opposed to having them.

This is also an incredibly misleading summary of what the real issue
was, as I just said in my previous email. We do credit reviewers,
routinely, and have for years. We have not reached an agreement on
whether or exactly how to credit them in the release notes. You may
think that there's no value in having your name show up in a commit
log message and that the release notes are the only thing that counts,
but I don't think everyone feels that way. I *still* get a kick out
of it every time somebody types my name into one of those messages.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/16/2014 4:32 AM, Simon Riggs wrote:
> On 15 December 2014 at 19:52, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> On 12/15/2014 11:27 AM, Robert Haas wrote:
>>> I feel like we used to be better at encouraging people to participate
>>> in the CF even if they were not experts, and to do the best they can
>>> based on what they did know. That was a helpful dynamic. Sure, the
>>> reviews weren't perfect, but more people helped, and reviewing some of
>>> the patch well and some of it in a more cursory manner is way better
>>> than reviewing none of it at all.
>>
>> Well, it was strongly expressed to me by a number of senior contributors
>> on this list and at the developer meeting that inexpert reviews were not
>> really wanted, needed or helpful. As such, I stopped recruiting new
>> reviewers (and, for that matter, doing them myself). I don't know if
>> the same goes for anyone else.
>
> I don't remember saying that, hearing it or agreeing with it and I
> don't agree with it now.
>

As a reviewer from long long ago, I can tell you that I wasn't sure I
was even helpful.

Things got busy at work, and I may not have been useful, and I annoyed
some people on pg-general, so I walked away for a while.

I have no knowledge of community-building so this might be a bad idea:

Perhaps levels (or titles) of reviewer's would be helpful. A freshman
reviewer is not expected to do anything useful, is expected to make
mistakes, and is there to learn.

The community votes and promotes them to junior (or whatever). They
know they are on the right track. A junior review is useful but maybe
not as complete as a senior reviewer. Maybe I'll aspire to work harder,
and maybe not, but at least I know what I'm doing is useful. If I never
get promoted, then I know, as well.

Freshmen know its ok to make mistakes. They know who they can contact
for help.

I think I like belts better (yellow, green, red, black).

I think this gives me two things:
1) permission to mess up
2) ability to measure myself

-Andy

On Tue, Dec 16, 2014 at 10:15 AM, Mark Cave-Ayland <
mark(dot)cave-ayland(at)ilande(dot)co(dot)uk> wrote:

>
> Well as I mentioned in my last email, practically all developers will
> rebase and run "make check" on their patched tree before submitting to
> the list.

​Even when this is true, and with people new to the project submitting
patches
I'm not sure it can be assumed, time passes and things can change between
submission​ and review. I've seen a fair number of "Needs rebase" comments
on patches, through no fault of the original submitter.

On 12/16/2014 08:48 AM, Mike Blackwell wrote:
> On Tue, Dec 16, 2014 at 10:15 AM, Mark Cave-Ayland
> <mark(dot)cave-ayland(at)ilande(dot)co(dot)uk <mailto:mark(dot)cave-ayland(at)ilande(dot)co(dot)uk>>wrote:
>
>
> Well as I mentioned in my last email, practically all developers will
> rebase and run "make check" on their patched tree before submitting to
> the list.
>
>
> ​Even when this is true, and with people new to the project submitting
> patches
> I'm not sure it can be assumed, time passes and things can change between
> submission​ and review. I've seen a fair number of "Needs rebase" comments
> on patches, through no fault of the original submitter.

This really should be taken care of by automation. Magnus's new system
will be a significant step forwards on enabling that.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> It's not like development on a patch series is difficult. You commit
> small fixes and changes, then you 'git rebase -i' and reorder them to
> apply to the appropriate changesets. Or you can do a 'rebase -i' and in
> 'e'dit mode make amendments to individual commits. Or you can commit
> 'fixup!'s that get auto-squashed.

I don't have a problem with this, but it's an independent consideration
for how a patch might be developed vs. what the main git repo looks
like. I don't think it makes sense to commit to the main repo a new
catalog table without any commands to manage it, nor to have a catalog
table which can be managed through the grammar but doesn't actually do
anything due to lacking the backend code for it.

Now, I fully agree that we want to continue to build features on top of
other features, but, in general, those need to be independently valuable
features when they are committed to the main repo (eg: WITH CHECK for
security barrier views went in first as it was independently useful, and
then RLS simply built on top of it).

> This is part of my grumbling about the use of git like it's still CVS.

Our git repo is actually readable and reasonably easy to follow and, for
my part, that's a great thing we have that most other projects don't.
That isn't to say that we shouldn't develop with smaller pieces, but I
tend to agree with Tom that it's actually easier (for me, at least) to
review a single, complete, patch than a bunch of independent ones which
build on each other. Perhaps that's my failing or a fault of my
processes, but I will actually sit and read through a patch in my email
client quite often. In general, I expect the code to compile and pass
'make check', those are necessary, of course, but detecting compile-time
problems is something the compiler is good at and I'm not. Thinking
about the impact of a new data structure, a given code block, or making
sure that all of the pieces of a new catalog row are set correctly are
things that the compiler isn't good at and is where a reviewer is most
valuable.

Pulling the code in and testing it by hand is useful, as is getting into
gdb and looking at the structures as they are manipulated, but I find it
extremely important to also actually review the *code*, which means
reading it and considering "are there places this patch needs to be
touching that it isn't? how will this other bit of code react to these
changes? does this code still look sane and like one person thought
through the whole code path? do the comments explain why things are
happening or do they just repeat what the code already says?", etc.

This goes back to the excellent point elsewhere on this thread that the
current documentation for reviewers is far too focused on the mechanical
bits which could basically be automated (in fact, I think Peter's
already got most of it automated..).

Thanks,

Stephen

David,

* David Rowley (dgrowleyml(at)gmail(dot)com) wrote:
> I'd just like to add something which might be flying below the radar of
> more senior people. There are people out there (ike me) working on
> PostgreSQL more for the challenge and perhaps the love of the product, who
> make absolutely zero money out of it. For these people getting credit where
> it's due is very important. I'm pretty happy with this at the moment and I
> can't imagine any situation where not crediting reviewers would be
> beneficial to anyone.

Thanks for this. One question which has been brought up in the past is
the specific "level" of credit. That you're happy with the credit
you've been given thus far is great as it happens to support the side
that I'm on. :D

However, could you quantify what, exactly, you feel is approrpiate
credit for reviewers and authors..? I'll intentionally omit the options
that have been presented in the past to try and avoid influencing your
response.

Thanks!

Stephen

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Including all of the other names of people who made important
> contributions, many of which consisted of reviewing, would make that
> release note item - and many others - really, really long, so I'm not
> in favor of that. Crediting reviewers is important, but so is having
> the release notes be readable.

Agreed.

> It has been proposed that we do a general list of people at the bottom
> of the release notes who helped review during that cycle. That would
> be less intrusive and possibly a good idea, but would we credit the
> people who did a TON of reviewing? Everyone who reviewed even one
> patch? Somewhere in between? Would committers be excluded because "we
> just expect them to help" or included because credit is important to
> established community members too? To what extent would this be
> duplicative of http://www.postgresql.org/community/contributors/ ?

I don't particularly like this idea.

> I'm not necessarily averse to doing something here, but the reason why
> nothing has happened has much more to do with the fact that it's hard
> to figure out exactly what the best thing would be than any idea that
> "we don't want to credit reviewers". We do want to credit reviewers,
> AND WE DO, as a quick look at 'git log' will speedily reveal.

Agreed.

Thanks,

Stephen

On Wed, Dec 17, 2014 at 12:03 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> David,
>
> * David Rowley (dgrowleyml(at)gmail(dot)com) wrote:
> > I'd just like to add something which might be flying below the radar of
> > more senior people. There are people out there (ike me) working on
> > PostgreSQL more for the challenge and perhaps the love of the product,
> who
> > make absolutely zero money out of it. For these people getting credit
> where
> > it's due is very important. I'm pretty happy with this at the moment and
> I
> > can't imagine any situation where not crediting reviewers would be
> > beneficial to anyone.
>
> Thanks for this. One question which has been brought up in the past is
> the specific "level" of credit. That you're happy with the credit
> you've been given thus far is great as it happens to support the side
> that I'm on. :D
>
> However, could you quantify what, exactly, you feel is approrpiate
> credit for reviewers and authors..? I'll intentionally omit the options
> that have been presented in the past to try and avoid influencing your
> response.
>
>
>
Mentioning them in list of contributors, for one.