Subject: commitfest.postgresql.org is no longer fit for purpose

From: Robert Haas <[email-masked]>
Date: Thu, 16 May 2024 14:30:03 -0400

Hi,

The original intent of CommitFests, and of commitfest.postgresql.org
by extension, was to provide a place where patches could be registered
to indicate that they needed to be reviewed, thus enabling patch
authors and patch reviewers to find each other in a reasonably
efficient way. I don't think it's working any more. I spent a good
deal of time going through the CommitFest this week, and I didn't get
through a very large percentage of it, and what I found is that the
status of the patches registered there is often much messier than can
be captured by a simple "Needs Review" or "Waiting on Author," and the
number of patches that are actually in need of review is not all that
large. For example, there are:

- patches parked there by a committer who will almost certainly do
something about them after we branch
- patches parked there by a committer who probably won't do something
about them after we branch, but maybe they will, or maybe somebody
else will, and anyway this way we at least run CI
- patches parked there by a committer who may well do something about
them before we even branch, because they're not actually subject to
the feature freeze
- patches that we've said we don't want but the author thinks we do
(sometimes i agree with the author, sometimes not)
- patches that have long-unresolved difficulties which the author
either doesn't know how to solve or is in no hurry to solve
- patches that have already been reviewed by multiple people, often
including several committers, and which have been updated multiple
times, but for one reason or another, not committed
- patches that actually do need to be reviewed

What's a bit depressing is that this last category is a relatively
small percentage of the total. If you'd like to sit down and review a
bunch of patches, you'll probably spend AT LEAST as much time trying
to identify which CommitFest entries are worth your time as you will
actually reviewing. I suspect you could easily spend 2 or 3 times as
much time finding things to review as actually reviewing them,
honestly. And the chances that you're going to find the things to
review that most need your attention are pretty much nil. You could
happen just by chance to discover a patch that was worth weeks of your
time to review, but you could also miss that patch forever amidst all
the clutter.

I think there are a couple of things that have led to this state of
affairs. First, we got tired of making people mad by booting their
stuff out of the CommitFest, so we mostly just stopped doing it, even
if it had 0% chance of being committed this CommitFest, and really
even if it had a 0% chance of being committed ever. Second, we added
CI, which means that there is positive value to registering the patch
in the CommitFest even if committing it is not in the cards. And those
things together have created a third problem, which is that the list
is now so long and so messy that even the CommitFest managers probably
don't manage to go through the whole thing thoroughly in a month.

So, our CommitFest application has turned into a patch tracker. IMHO,
patch trackers intrinsically tend to suck, because they fill up with
garbage that nobody cares about, and nobody wants to do the colossal
amount of work that it takes to maintain them. But our patch tracker
sucks MORE, because it's not even intended to BE a general-purpose
patch tracker. I'm not saying that replacing it with (let me show how
old I am) bugzilla or whatever the hip modern equivalent of that may
be these days is the right thing to do, but it's probably worth
considering. If we decide to roll our own, that might be OK too, but
we have to come up with some way of organizing this stuff that's
better than what we have today, some way that actually lets you find
the stuff that you care about.

To give just one example that I think highlights the issues pretty
well, consider the "Refactoring" section of the current CommitFest.
There are 24 patches in there, and 13 of them are by committers. Now,
maybe some of those patches are things that the committer really wants
someone else to review, e.g.
https://commitfest.postgresql.org/48/3998/ seems like it might be
that. On the other hand, that one could also just be an idea Thomas
had that he doesn't really intend to pursue even if the reviews are
absolutely glowing, so maybe it's not worth spending time on after
all. Then there are things that are probably 100% likely to get
committed unless somebody objects, so I shouldn't bother looking at
them unless I want to object, e.g.
https://commitfest.postgresql.org/48/4939/ seems like it's probably
that. And, also, regardless of authorship, some of these patches have
already had a great deal of discussion, and some have had none, and
you can sort of tell that from looking at the time the patch was
created vs. the last activity, but it's really not that obvious. So
overall it's just really unclear where to spend time.

I wonder what ideas people have for improving this situation. I doubt
that there's any easy answer that just makes the problem go away --
keeping large groups of people organized is a tremendously difficult
task under pretty much all circumstances, and the fact that, in this
context, nobody's really the boss, makes it a whole lot harder. But I
also feel like what we're doing right now can't possibly be the best
that we can do.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: "David G. Johnston" <[email-masked]>
Date: Thu, 16 May 2024 12:13:17 -0700

> them before we even branch, because they're not actually subject to

If a committer has a patch in the CF that is going to be committed in the
future unless there is outside action those should be put under a "Pending
Commit" status.

> times, but for one reason or another, not committed

Use the same software but a different endpoint - Collaboration.  Or even
the same endpoint just add an always open slot named "Work In Process"
(WIP).  If items can be moved from there to another open or future
commitfest slot so much the better.

If we can distinguish between needs to be reviewed by a committer
(commitfest dated slots - bimonthlies) and reviewed by someone other than
the author (work in process slot) that should help everyone.  Of course,
anyone is welcome to review a patch that has been marked ready to commit.
I suppose "ready to commit" already sorta does this without the need for
WIP but a quick sanity check would be that ready to commit shouldn't (not
mustn't) be seen in WIP and needs review shouldn't be seen in the
bimonthlies.  A needs review in WIP means that the patch has been seen by a
committer and sent back for more work but that the scope and intent are
such that it will make it into the upcoming major release.  Or is something
like a bug fix that just goes right into the bimonthly instead of starting
out as a WIP item.

> even if it had a 0% chance of being committed ever.

Those likely never get out of the new WIP slot discussed above.  Your patch
tracker basically.  And there should be less angst in moving something in
the bimonthly into WIP rather than dropping it outright.  There is
discussion to be had regarding WIP/patch tracking should we go this
direction but even if it is just movIng clutter from one room to another
there seems like a clear benefit and need to tighten up what it means to be
in the bimonthly slot - to have qualifications laid out for a patch to earn
its way there, either by effort (authored and reviewed) or need (i.e., bug
fixes), or, realistically, being authored by a committer and being mostly
trivial in nature.

> Second, we added
> in the CommitFest even if committing it is not in the cards.

The new slot retains this benefit.

And those
> don't manage to go through the whole thing thoroughly in a month.

The new slot wouldn't be subject to this.

We'll still have a problem with too many WIP patches and not enough ability
or desire to resolve them.  But setting a higher bar to get onto the
bimonthly slot while still providing a place for collaboration is a step
forward that configuring technology can help with.  As for WIP, maybe
adding thumbs-up and thumbs-down support tracking widgets will help draw
attention to more desired things.

David J.

---

From: Maciek Sakrejda <[email-masked]>
Date: Thu, 16 May 2024 12:25:38 -0700

Thanks for raising this. As someone who is only modestly familiar with
Postgres internals or even C, but would still like to contribute through
review, I find the current process of finding a suitable patch both tedious
and daunting. The Reviewing a Patch article on the wiki [0] says reviews
like mine are still welcome, but it's hard to get started. I'd love to see
this be more approachable.

Thanks,
Maciek

[0]: https://wiki.postgresql.org/wiki/Reviewing_a_Patch

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 16 May 2024 21:30:10 +0200

> On 16 May 2024, at 20:30, Robert Haas <[email-masked]> wrote:

> efficient way. I don't think it's working any more.

But which part is broken though, the app, our commitfest process and workflow
and the its intent, or our assumption that we follow said process and workflow
which may or may not be backed by evidence?  IMHO, from being CMF many times,
there is a fair bit of the latter, which excacerbates the problem.  This is
harder to fix with more or better software though. 

> I spent a good deal of time going through the CommitFest this week

And you deserve a big Thank You for that.

--
Daniel Gustafsson

---

From: Melanie Plageman <[email-masked]>
Date: Thu, 16 May 2024 16:46:16 -0400

> else will, and anyway this way we at least run CI

-- snip --

> patch tracker.

I was reflecting on why a general purpose patch tracker sounded
appealing to me, and I realized that, at least at this time of year, I
have a few patches that really count as "waiting on author" that I
know I need to do additional work on before they need more review but
which aren't currently my top priority. I should probably simply
withdraw and re-register them. My justification was that I'll lose
them if I don't keep them in the commitfest app. But, I could just,
you know, save them somewhere myself instead of polluting the
commitfest app with them. I don't know if others are in this
situation. Anyway, I'm definitely currently guilty of parking.

- Melanie

---

From: Jacob Champion <[email-masked]>
Date: Thu, 16 May 2024 13:50:22 -0700

On Thu, May 16, 2024 at 12:14 PM David G. Johnston
<[email-masked]> wrote:
> Those likely never get out of the new WIP slot discussed above.  Your patch tracker basically.  And there should be less angst in moving something in the bimonthly into WIP rather than dropping it outright.  There is discussion to be had regarding WIP/patch tracking should we go this direction but even if it is just movIng clutter from one room to another there seems like a clear benefit

Yeah, IMO we're unlikely to get around the fact that it's a patch
tracker -- even if patch trackers inherently tend to suck, as Robert
put it, they tend to have lots of value too. May as well embrace the
need for a tracker and make it more helpful.

> We'll still have a problem with too many WIP patches and not enough ability or desire to resolve them.  But setting a higher bar to get onto the bimonthly slot while still providing a place for collaboration is a step forward that configuring technology can help with.

+1. I think _any_ way to better communicate "what the author needs
right now" would help a lot.

> As for WIP, maybe adding thumbs-up and thumbs-down support tracking widgets will help draw attention to more desired things.

Personally I'd like a way to gauge general interest without
introducing a voting system. "Stars", if you will, rather than
"thumbs". In the context of the CF, it's valuable to me as an author
that you care about what I'm trying to do; if you don't like my
implementation, that's what reviews on the list are for. And I see no
way that the meaning of a thumbs-down button wouldn't degrade
immediately.

I have noticed that past proposals for incremental changes to the CF
app (mine and others') are met with a sort of resigned inertia --
sometimes disagreement, but more often "meh, sounds all right, maybe".
Maybe my suggestions are just meh, and that's fine. But if we can't
tweak small things as we go -- and be generally okay with trying and
reverting failed experiments sometimes -- frustrations are likely to
pile up until someone writes another biyearly manifesto thread.

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Thu, 16 May 2024 16:54:04 -0400

> situation. Anyway, I'm definitely currently guilty of parking.

It's also nice that the CF app will run CI for you, so at least
you can keep the patch building if you're so inclined.

David J. had a suggestion for this too upthread, which was to create a
separate slot for WIP patches that isn't one of the dated CF slots.

It's hard to argue that such patches need to be in "the CF app" at
all, if you're not actively looking for review.  But the CI runs
and the handy per-author patch status list make the app very tempting
infrastructure for parked patches.  Maybe we could have a not-the-CF
app that offers those amenities?

			regards, tom lane

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 16 May 2024 23:35:45 +0200

> situation. Anyway, I'm definitely currently guilty of parking.

I don't have a problem with that at all.  It's pretty understandable 
that many patches are parked between say April and July.

The key is the keep the status up to date.

If we have 890 patches in Waiting for Author and 100 patches in Ready 
for Committer and 10 patches in Needs Review, and those 10 patches are 
actually reviewable, then that's good.  There might need to be a 
"background process" to make sure those 890 waiting patches are not 
parked forever and those 100 patches actually get committed sometime, 
but in principle this is the system working.

The problem is if we have 180 patches in Needs Review, and only 20 are 
really actually ready to be reviewed.  And a second-order problem is 
that if you already know that this will be the case, you give up before 
even looking.

---

From: Tom Lane <[email-masked]>
Date: Thu, 16 May 2024 17:46:27 -0400

> that if you already know that this will be the case, you give up before 

Right, so what can we do about that?  Does Needs Review state need to
be subdivided, and if so how?

If it's just that a patch should be in some other state altogether,
we should simply encourage people to change the state as soon as they
discover that.  I think the problem is not so much "90% are in the
wrong state" as "each potential reviewer has to rediscover that".

At this point it seems like there's consensus to have a "parking"
section of the CF app, separate from the time-boxed CFs, and I hope
somebody will go make that happen.  But I don't think that's our only
issue, so we need to keep thinking about what should be improved.

			regards, tom lane

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 17 May 2024 00:03:30 +0200

> be subdivided, and if so how?

Maybe a new state "Unclear".  Then a commitfest manager, or someone like 
Robert just now, can more easily power through the list and set 
everything that's doubtful to Unclear, with the understanding that the 
author can set it back to Needs Review to signal that they actually 
think it's ready.  Or, as a variant of what someone else was saying, 
just set all patches that carry over from March to July as Unclear.  Or 
something like that.

I think, if we consider the core mission of the commitfest app, we need 
to be more protective of the Needs Review state.

I have been, from time to time, when I'm in commitfest management mode, 
aggressive in setting entries back to Waiting on Author, but that's not 
always optimal.

So a third status that encompasses the various other situations like 
maybe forgotten by author, disagreements between author and reviewer, 
process difficulties, needs some senior developer intervention, etc. 
could be helpful.

This could also help scale the commitfest manager role, because then the 
head CFM could have like three helpers and just tell them, look at all 
the "Unclear" ones and help resolve them.

---

From: Robert Haas <[email-masked]>
Date: Thu, 16 May 2024 22:26:13 -0400

> be subdivided, and if so how?

It doesn't really matter how many states we have if they're not set accurately.

> issue, so we need to keep thinking about what should be improved.

I do *emphatically* think we need a parking lot. And a better
integration between commitfest.postgresql.org and the cfbot, too. It's
just ridiculous that more work hasn't been put into this. I'm not
faulting Thomas or anyone else -- I mean, it's not his job to build
the infrastructure the project needs any more than it is anyone else's
-- but for a project of the size and importance of PostgreSQL to take
years to make minor improvements to this kind of critical
infrastructure is kind of nuts. If we don't have enough volunteers,
let's go recruit some more and promise them cookies.

I think you're overestimating the extent to which the problem is "we
don't reject enough patches". That *is* a problem, but it seems we
have also started rejecting some patches that just never really got
much attention for no particularly good reason, while letting other
things linger that didn't really get that much more attention, or
which were objectively much worse ideas. I think that one place where
the process is breaking down is in the tacit assumption that the
person who wrote the patch wants to get it committed. In some cases,
people park things in the CF for CI runs without a strong intention of
pursuing them; in other cases, the patch author is in no kind of rush.

I think we need to move to a system where patches exist independent of
a CommitFest, and get CI run on them unless they get explicitly closed
or are too inactive or some other criterion that boils down to nobody
cares any more. Then, we need to get whatever subset of those patches
need to be reviewed in a particular CommitFest added to that
CommitFest. For example, imagine that the CommitFest is FORCIBLY empty
until a week before it starts. You can still register patches in the
system generally, but that just means they get CI runs, not that
they're scheduled to be reviewed. A week before the CommitFest,
everyone who has a patch registered in the system that still applies
gets an email saying "click here if you think this patch should be
reviewed in the upcoming CommitFest -- if you don't care about the
patch any more or it needs more work before other people review it,
don't click here". Then, the CommitFest ends up containing only the
things where the patch author clicked there during that week.

For bonus points, suppose we make it so that when you click the link,
it takes you to a box where you can type in a text comment that will
display in the app, explaining why your patch needs review: "Robert
says the wire protocol changes in this patch are wrong, but I think
he's full of hot garbage and want a second opinion!" (a purely
hypothetical example, I'm sure) If you put in a comment like this when
you register your patch for the CommitFest, it gets a sparkly gold
border and floats to the top of the list, or we mail you a Kit-Kat
bar, or something. I don't know.

The point is - we need a much better signal to noise ratio here. I bet
the number of patches in the CommitFest that actually need review is
something like 25% of the total. The rest are things that are just
parked there by a committer, or that the author doesn't care about
right now, or that are already being actively discussed, or where
there's not a clear way forward. We could create new statuses for all
of those states - "Parked", "In Hibernation," "Under Discussion," and
"Unclear" - but I think that's missing the point. What we really want
is to not see that stuff in the first place. It's a CommitFest, not
once-upon-a-time-I-wrote-a-patch-Fest.

---

From: Yasir <[email-masked]>
Date: Fri, 17 May 2024 12:19:29 +0500

> this be more approachable.
I totally agreed with Maciek. Its really hard for even an experience
developer to become a PG contributor or be able to contribute effectively.

---

From: Heikki Linnakangas <[email-masked]>
Date: Fri, 17 May 2024 10:32:54 +0300

> bar, or something. I don't know.

Dunno about having to click a link or sparkly gold borders, but +1 on 
having a free-form text box for notes like that. Things like "cfbot says 
this has bitrotted" or "Will review this after other patch this depends 
on". On the mailing list, notes like that are both noisy and easily lost 
in the threads. But as a little free-form text box on the commitfest, it 
would be handy.

One risk is that if we start to rely too much on that, or on the other 
fields in the commitfest app for that matter, we de-value the mailing 
list archives. I'm not too worried about it, the idea is that the 
summary box just summarizes what's already been said on the mailing 
list, or is transient information like "I'll get to this tomorrow" 
that's not interesting to archive.

> once-upon-a-time-I-wrote-a-patch-Fest.

Yeah, I'm also skeptical of adding new categories or statuses to the 
commitfest.

I sometimes add patches to the commitfest that are not ready to be 
committed, because I want review on the general idea or approach, before 
polishing the patch to final state. That's also a fine use of the 
commitfest app. The expected workflow is to get some review on the 
patch, and then move it back to Waiting on Author.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 17 May 2024 10:15:39 +0200

On Fri, 17 May 2024 at 06:58, Peter Eisentraut <[email-masked]> wrote:
> Yeah, some fine-tuning might be required.  But I would be wary of
> over-designing too many new states at this point.  I think the key idea
> is that there ought to be a state that communicates "needs attention
> from someone other than author, reviewer, or committer".

+1 on adding a new state like this. I think it would make sense for
the author to request additional input, but I think it would need two
states, something like "Request for new reviewer" and "Request for new
committer". Just like authors disappear sometimes after a driveby
patch submission, it's fairly common too imho for reviewers or
committers to disappear after a driveby review. Having a way for an
author to mark a patch as such would be helpful, both to the author,
and to reviewers/committers looking to do help some patch out.

On Fri, 17 May 2024 at 09:33, Heikki Linnakangas <[email-masked]> wrote:
> Things like "cfbot says

+many on the free form text box

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 17 May 2024 10:47:08 +0200

> On 17 May 2024, at 09:32, Heikki Linnakangas <[email-masked]> wrote:

> On the mailing list, notes like that are both noisy and easily lost in the threads. But as a little free-form text box on the commitfest, it would be handy.

On a similar note, I have in the past suggested adding a free-form textfield to
the patch submission form for the author to give a short summary of what the
patch does/adds/requires etc.  While the thread contains all of this, it's
likely quite overwhelming for many in general and new contributors in
particular.  A short note, on purpose limited to ~500 chars or so to not allow
mailinglist post copy/paste, could be helpful there I think (I've certainly
wanted one, many times over, especially when doing CFM).

> One risk is that if we start to rely too much on that, or on the other fields in the commitfest app for that matter, we de-value the mailing list archives. I'm not too worried about it, the idea is that the summary box just summarizes what's already been said on the mailing list, or is transient information like "I'll get to this tomorrow" that's not interesting to archive.

One way to ensure we capture detail could be if the system would send an
automated email to the thread summarizing the entry when it's marked as
"committed"?

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 17 May 2024 10:58:08 +0200

> On 17 May 2024, at 00:03, Peter Eisentraut <[email-masked]> wrote:

> I think, if we consider the core mission of the commitfest app, we need to be more protective of the Needs Review state.

IMHO this is a very very good summary of what we should focus on with this
work.

--
Daniel Gustafsson

---

From: Chris Travers <[email-masked]>
Date: Fri, 17 May 2024 16:19:01 +0700

I think there are actually a number of factors that make this much harder.

> > once-upon-a-time-I-wrote-a-patch-Fest.

Yeah this is a problem.

I think in cases here something is in hibernation or unclear it really
should be "returned with feedback."  There's really nothing stopping
someone from learning from the experience and resubmitting an improved
version.

I think under discussion is also rather unclear.  The current statuses
already cover this sort of thing (waiting for author and waiting for
review).  Maybe we could improve the categories here but it is
important to note whether the author or a reviewer is expected to take
the next step.

If the author doesn't respond within a period of time (let's say 30
days), I think we can just say "returned with feedback."

Since you can already attach older threads to a commitfest entry, it
seems to me that we ought to be more aggressive with "returned with
feedback" and note that this doesn't necessarily mean "rejected" which
is a separate status which we rarely use.

---

From: Joe Conway <[email-masked]>
Date: Fri, 17 May 2024 08:19:41 -0400

On 5/16/24 22:26, Robert Haas wrote:
> For example, imagine that the CommitFest is FORCIBLY empty

100% agree. This is in line with what I suggested on an adjacent part of 
the thread.

> there's not a clear way forward.

I think there is another case that no one talks about, but I'm sure 
exists, and that I am not the only one guilty of thinking this way.

Namely, the week before commitfest I don't actually know if I will have 
the time during that month, but I will make sure my patch is in the 
commitfest just in case I get a few clear days to work on it. Because if 
it isn't there, I can't take advantage of those "found" hours.

> We could create new statuses for all of those states - "Parked", "In 
> Hibernation," "Under Discussion," and "Unclear" - but I think that's 
> missing the point. What we really want is to not see that stuff in 
> the first place. It's a CommitFest, not

+1

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Dmitry Dolgov <[email-masked]>
Date: Sun, 19 May 2024 11:37:19 +0200

> On Thu, May 16, 2024 at 02:30:03PM -0400, Robert Haas wrote:
>

There are lots of good takes on this in the thread. It also makes clear what's
at stake -- as Melanie pointed out with the patch about EXPLAIN for parallel
bitmap heap scan, we're loosing potential contributors for no reasons. But I'm
a bit concerned about what are the next steps: if memory serves, every couple
of years there is a discussion about everything what goes wrong with the review
process, commitfests, etc. Yet to my (admittedly limited) insight into the
community, not many things have changed due to those discussions. How do we
make sure this time it will be different?

It is indeed tremendously difficult to self organize, so maybe it's worth to
volunteer a group of people to work out details of one or two proposals,
answering the question "how to make it better?". As far as I understand, the
community already has a similar experience. Summarizing this thread,
there seems to be following dimensions to look at:

* What is the purpose of CF and how to align it better with the community
  goals.

  "CommitFest" here means both the CF tool and the process behind it. So far
  the discussion was evolving around the state machine for each individual CF
  item as well as the whole CF cycle. At the end of the day perhaps a list of
  pairs (item, status) is not the best representation, probably more filters
  have to be considered (e.g. implementing a workflow "give me all the items,
  updated in the last month with the last reply being from the patch author").

* How to synchronize the mailing list with CF content.

  The entropy of CF content grows over time, making it less efficient. For
  especially old threads it's even more visible. How to reduce the entropy
  without scaring new contributors away?

* How to deal with review scalability bottleneck.

  An evergreen question. PostgreSQL is getting more popular and, as stated in
  diverse research whitepapers, the amount of contribution grows as a power
  law, where the number of reviewers grows at best sub-linearly (limited by the
  velocity of knowledge sharing). I agree with Andrey on this, the only
  way I see to handle this is to scale CF management efforts.

* What are the UX gaps of CF tool?

  There seems to be some number of improvements that could make work with CF
  tool more frictionless.

What I think wasn't discussed yet in details is the question of motivation.
Surely, it would be great to have a process that will introduce as less burden
as possible. But giving more motivation to follow the process / use the tool is
as important. What motivates folks to review patches, figure out status of a
complicated patch thread, maintain a list of open items, etc?

---

From: Joe Conway <[email-masked]>
Date: Sun, 19 May 2024 10:42:01 -0400

>    way I see to handle this is to scale CF management efforts.

The number of items tracked are surely growing, but I am not sure I 
would call it exponential -- see attached