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

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

Op 5/16/24 om 20:30 schreef Robert Haas:
> 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

Hi,

Perhaps it would be an idea to let patches 'expire' automatically unless 
they are 'rescued' (=given another year)  by committer or commitfest 
manager (or perhaps a somewhat wider group - but not too many). 
Expiration after, say, one year should force patch-authors to mount a 
credible defense for his/her patch to either get his work rescued or 
reinstated/resubmitted.

Just a thought that has crossed my mind already a few times. It's not 
very sympathetic but it might work keep the list smaller.

Erik Rijkers

> On 16 May 2024, at 20:30, Robert Haas <robertmhaas@gmail.com> wrote:

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

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

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 16 May 2024, at 20:30, Robert Haas <robertmhaas@gmail.com> wrote:
>> 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.

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

Yeah.  I think that Robert put his finger on a big part of the
problem, which is that punting a patch to the next CF is a lot
easier than rejecting it, particularly for less-senior CFMs
who may not feel they have the authority to say no (or at
least doubt that the patch author would accept it).  It's hard
even for senior people to get patch authors to take no for an
answer --- I know I've had little luck at it --- so maybe that
problem is inherent.  But a CF app full of patches that are
unlikely ever to go anywhere isn't helpful.

It's also true that some of us are abusing the process a bit.
I know I frequently stick things into the CF app even if I intend
to commit them pretty darn soon, because it's a near-zero-friction
way to run CI on them, and I'm too lazy to learn how to do that
otherwise.  I like David's suggestion of a "Pending Commit"
status, or maybe I should just put such patches into RfC state
immediately?  However, short-lived entries like that don't seem
like they're a big problem beyond possibly skewing the CF statistics
a bit.  It's the stuff that keeps hanging around that seems like
the core of the issue.

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

> And you deserve a big Thank You for that.

+ many

            regards, tom lane

On 5/16/24 15:47, Tom Lane wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 16 May 2024, at 20:30, Robert Haas <robertmhaas@gmail.com> wrote:
>>> 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.
> 
>> 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. 
> 
> Yeah.  I think that Robert put his finger on a big part of the
> problem, which is that punting a patch to the next CF is a lot
> easier than rejecting it, particularly for less-senior CFMs
> who may not feel they have the authority to say no (or at
> least doubt that the patch author would accept it).

Maybe we should just make it a policy that *nothing* gets moved forward 
from commitfest-to-commitfest and therefore the author needs to care 
enough to register for the next one?

>>> I spent a good deal of time going through the CommitFest this week
> 
>> And you deserve a big Thank You for that.
> 
> + many

+1 agreed

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

On Thu, May 16, 2024 at 2:30 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> 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

-- snip --

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

On Thu, May 16, 2024 at 12:14 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> Those likely never get out of the new WIP slot discussed above.  Your patch tracker basically.  And there should be
lessangst in moving something in the bimonthly into WIP rather than dropping it outright.  There is discussion to be
hadregarding WIP/patch tracking should we go this direction but even if it is just movIng clutter from one room to
anotherthere 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
higherbar to get onto the bimonthly slot while still providing a place for collaboration is a step forward that
configuringtechnology 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

Melanie Plageman <melanieplageman@gmail.com> writes:
> 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.

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

On Thu, May 16, 2024 at 1:46 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> 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.

Me too -- it's really, really useful. I think there's probably a
better way the app could help us mark it as parked, as opposed to
getting rid of parking. Similar to how people currently use the
Reviewer field as a personal TODO list... it might be nice to
officially separate the ideas a bit.

--Jacob

On Thu, May 16, 2024 at 1:31 PM Joe Conway <mail@joeconway.com> wrote:
> Maybe we should just make it a policy that *nothing* gets moved forward
> from commitfest-to-commitfest and therefore the author needs to care
> enough to register for the next one?

I think that's going to severely disadvantage anyone who doesn't do
this as their day job. Maybe I'm bristling a bit too much at the
wording, but not having time to shepherd a patch is not the same as
not caring.

--Jacob

Hi,

On 5/16/24 4:31 PM, Joe Conway wrote:
>> Yeah.  I think that Robert put his finger on a big part of the
>> problem, which is that punting a patch to the next CF is a lot
>> easier than rejecting it, particularly for less-senior CFMs
>> who may not feel they have the authority to say no (or at
>> least doubt that the patch author would accept it).
> 
> Maybe we should just make it a policy that *nothing* gets moved forward 
> from commitfest-to-commitfest and therefore the author needs to care 
> enough to register for the next one?
>

Or at least nothing get moved forward from March.

Spending time on a patch during a major version doesn't mean that you 
have time to do the same for the next major version.

That way July could start "clean" with patches people are interested in 
and willing to maintain during the next year.

Also, it is a bit confusing that f.ex.

  https://commitfest.postgresql.org/48/

already shows 40 patches as Committed...

So, having some sort of "End of Development" state in general would be good.

Best regards,
  Jesper

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> ... Similar to how people currently use the
> Reviewer field as a personal TODO list... it might be nice to
> officially separate the ideas a bit.

Oh, that's an independent pet peeve of mine.  Usually, if I'm
looking over the CF list for a patch to review, I skip over ones
that already show an assigned reviewer, because I don't want to
step on that person's toes.  But it seems very common to put
one's name down for review without any immediate intention of
doing work.  Or to do a review and wander off, leaving the patch
apparently being tended to but not really.  (And I confess I'm
occasionally guilty of both things myself.)

I think it'd be great if we could separate "I'm actively reviewing
this" from "I'm interested in this".  As a bonus, adding yourself
to the "interested" list would be a fine proxy for the thumbs-up
or star markers mentioned upthread.

If those were separate columns, we could implement some sort of
aging scheme whereby somebody who'd not commented for (say)
a week or two would get quasi-automatically moved from the "active
reviewer" column to the "interested" column, whereupon it wouldn't
be impolite for someone else to sign up for active review.

            regards, tom lane

On 5/16/24 16:57, Jacob Champion wrote:
> On Thu, May 16, 2024 at 1:31 PM Joe Conway <mail@joeconway.com> wrote:
>> Maybe we should just make it a policy that *nothing* gets moved forward
>> from commitfest-to-commitfest and therefore the author needs to care
>> enough to register for the next one?
> 
> I think that's going to severely disadvantage anyone who doesn't do
> this as their day job. Maybe I'm bristling a bit too much at the
> wording, but not having time to shepherd a patch is not the same as
> not caring.

Maybe the word "care" was a poor choice, but forcing authors to think 
about and decide if they have the "time to shepherd a patch" for the 
*next CF* is exactly the point. If they don't, why clutter the CF with it.

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

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> On Thu, May 16, 2024 at 1:31 PM Joe Conway <mail@joeconway.com> wrote:
>> Maybe we should just make it a policy that *nothing* gets moved forward
>> from commitfest-to-commitfest and therefore the author needs to care
>> enough to register for the next one?

> I think that's going to severely disadvantage anyone who doesn't do
> this as their day job. Maybe I'm bristling a bit too much at the
> wording, but not having time to shepherd a patch is not the same as
> not caring.

Also, I doubt that there are all that many patches that have simply
been abandoned by their authors.  Our problem is the same as it's
been for many years: not enough review person-power, rather than
not enough patches.  So I think authors would just jump through that
hoop, or enough of them would that it wouldn't improve matters.

            regards, tom lane

On 5/16/24 16:48, Magnus Hagander wrote:
> On Thu, May 16, 2024 at 10:46 PM Melanie Plageman 
>     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.
> 
> 
> One thing I think we've talked about before (but not done) is to 
> basically have a CF called "parking lot", where you can park patches 
> that aren't active in a commitfest  but you also don't want to be dead. 
> It would probably also be doable to have the cf bot run patches in that 
> commitfest as well as the current one, if that's what people are using 
> it for there.

+1

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

On Thu, May 16, 2024 at 2:06 PM Joe Conway <mail@joeconway.com> wrote:
> Maybe the word "care" was a poor choice, but forcing authors to think
> about and decide if they have the "time to shepherd a patch" for the
> *next CF* is exactly the point. If they don't, why clutter the CF with it.

Because the community regularly encourages new patch contributors to
park their stuff in it, without first asking them to sign on the
dotted line and commit to the next X months of their free time. If
that's not appropriate, then I think we should decide what those
contributors need to do instead, rather than making a new bar for them
to clear.

--Jacob

On 5/16/24 17:24, Jacob Champion wrote:
> On Thu, May 16, 2024 at 2:06 PM Joe Conway <mail@joeconway.com> wrote:
>> Maybe the word "care" was a poor choice, but forcing authors to think
>> about and decide if they have the "time to shepherd a patch" for the
>> *next CF* is exactly the point. If they don't, why clutter the CF with it.
> 
> Because the community regularly encourages new patch contributors to
> park their stuff in it, without first asking them to sign on the
> dotted line and commit to the next X months of their free time. If
> that's not appropriate, then I think we should decide what those
> contributors need to do instead, rather than making a new bar for them
> to clear.

If no one, including the author (new or otherwise) is interested in 
shepherding a particular patch, what chance does it have of ever getting 
committed?

IMHO the probability is indistinguishable from zero anyway.

Perhaps we should be more explicit to new contributors that they need to 
either own their patch through the process, or convince someone to do it 
for them.

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

On 16.05.24 22:46, Melanie Plageman wrote:
> 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.

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.

On Thu, May 16, 2024 at 2:29 PM Joe Conway <mail@joeconway.com> wrote:
> If no one, including the author (new or otherwise) is interested in
> shepherding a particular patch, what chance does it have of ever getting
> committed?

That's a very different thing from what I think will actually happen, which is

- new author posts patch
- community member says "use commitfest!"
- new author registers patch
- no one reviews it
- patch gets automatically booted
- community member says "register it again!"
- new author says ಠ_ಠ

Like Tom said upthread, the issue isn't really that new authors are
somehow uninterested in their own patches.

--Jacob

On 16.05.24 23:04, Tom Lane wrote:
> I think it'd be great if we could separate "I'm actively reviewing
> this" from "I'm interested in this".  As a bonus, adding yourself
> to the "interested" list would be a fine proxy for the thumbs-up
> or star markers mentioned upthread.
> 
> If those were separate columns, we could implement some sort of
> aging scheme whereby somebody who'd not commented for (say)
> a week or two would get quasi-automatically moved from the "active
> reviewer" column to the "interested" column, whereupon it wouldn't
> be impolite for someone else to sign up for active review.

Yes, I think some variant of this could be quite useful.

On 16.05.24 23:06, Joe Conway wrote:
> On 5/16/24 16:57, Jacob Champion wrote:
>> On Thu, May 16, 2024 at 1:31 PM Joe Conway <mail@joeconway.com> wrote:
>>> Maybe we should just make it a policy that *nothing* gets moved forward
>>> from commitfest-to-commitfest and therefore the author needs to care
>>> enough to register for the next one?
>>
>> I think that's going to severely disadvantage anyone who doesn't do
>> this as their day job. Maybe I'm bristling a bit too much at the
>> wording, but not having time to shepherd a patch is not the same as
>> not caring.
> 
> Maybe the word "care" was a poor choice, but forcing authors to think 
> about and decide if they have the "time to shepherd a patch" for the 
> *next CF* is exactly the point. If they don't, why clutter the CF with it.

Objectively, I think this could be quite effective.  You need to prove 
your continued interest in your project by pressing a button every two 
months.

But there is a high risk that this will double the annoyance for 
contributors whose patches aren't getting reviews.  Now, not only are 
you being ignored, but you need to prove that you're still there every 
two months.

On Thu, May 16, 2024 at 2:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Oh, that's an independent pet peeve of mine.  Usually, if I'm
> looking over the CF list for a patch to review, I skip over ones
> that already show an assigned reviewer, because I don't want to
> step on that person's toes.  But it seems very common to put
> one's name down for review without any immediate intention of
> doing work.  Or to do a review and wander off, leaving the patch
> apparently being tended to but not really.  (And I confess I'm
> occasionally guilty of both things myself.)

Yep, I do the same thing (and have the same pet peeve).

> I think it'd be great if we could separate "I'm actively reviewing
> this" from "I'm interested in this".  As a bonus, adding yourself
> to the "interested" list would be a fine proxy for the thumbs-up
> or star markers mentioned upthread.
>
> If those were separate columns, we could implement some sort of
> aging scheme whereby somebody who'd not commented for (say)
> a week or two would get quasi-automatically moved from the "active
> reviewer" column to the "interested" column, whereupon it wouldn't
> be impolite for someone else to sign up for active review.

+1!

--Jacob

Peter Eisentraut <peter@eisentraut.org> writes:
> 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.

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

> On 16 May 2024, at 23:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Pulling in the information from the CFBot regarding test failures and whether
the patch applies at all, and automatically altering the state to WOA for at
least the latter category would be nice.  There's likely to be more analysis we
can do on the thread to measure "activity/hotness", but starting with the
simple boolean data we already have could be a good start.

--
Daniel Gustafsson

On 5/16/24 17:36, Jacob Champion wrote:
> On Thu, May 16, 2024 at 2:29 PM Joe Conway <mail@joeconway.com> wrote:
>> If no one, including the author (new or otherwise) is interested in
>> shepherding a particular patch, what chance does it have of ever getting
>> committed?
> 
> That's a very different thing from what I think will actually happen, which is
> 
> - new author posts patch
> - community member says "use commitfest!"

Here is where we should point them at something that explains the care 
and feeding requirements to successfully grow a patch into a commit.

> - new author registers patch
> - no one reviews it
> - patch gets automatically booted

Part of the care and feeding instructions should be a warning regarding 
what happens if you are unsuccessful in the first CF and still want to 
see it through.

> - community member says "register it again!"
> - new author says ಠ_ಠ

As long as this is not a surprise ending, I don't see the issue.

> Like Tom said upthread, the issue isn't really that new authors are
> somehow uninterested in their own patches.

First, some of them objectively are uninterested in doing more than 
dropping a patch over the wall and never looking back. But admittedly 
that is not too often.

Second, I don't think a once every two months effort in order to 
register continuing interest is too much to ask.

And third, if we did something like Magnus' suggestion about a CF 
parking lot, the process would be even more simple.

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

Daniel Gustafsson <daniel@yesql.se> writes:
> Pulling in the information from the CFBot regarding test failures and whether
> the patch applies at all, and automatically altering the state to WOA for at
> least the latter category would be nice.

+1.  There are enough intermittent test failures that I don't think
changing state for that would be good, but patch apply failure is
usually persistent.

I gather that the CFBot is still a kluge that is web-scraping the CF
app rather than being actually integrated with it, and that there are
plans to make that better that haven't been moving fast.  Probably
that would have to happen first, but there would be a lot of benefit
from having the info flow be two-way.

            regards, tom lane

On 16.05.24 23:46, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> 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.
> 
> Right, so what can we do about that?  Does Needs Review state need to
> 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.

Peter Eisentraut <peter@eisentraut.org> writes:
> On 16.05.24 23:46, Tom Lane wrote:
>> Right, so what can we do about that?  Does Needs Review state need to
>> be subdivided, and if so how?

> Maybe a new state "Unclear". ...

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

Yeah, makes sense.

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

Hmm, "forgotten by author" seems to generally turn into "this has been
in WOA state a long time".  Not sure we have a problem representing
that, only with a process for eventually retiring such entries.
Your other three examples all sound like "needs senior developer
attention", which could be a helpful state that's distinct from "ready
for committer".  It's definitely not the same as "Unclear".

            regards, tom lane

On Thu, May 16, 2024 at 5:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Jacob Champion <jacob.champion@enterprisedb.com> writes:
> > ... Similar to how people currently use the
> > Reviewer field as a personal TODO list... it might be nice to
> > officially separate the ideas a bit.
>
> Oh, that's an independent pet peeve of mine.  Usually, if I'm
> looking over the CF list for a patch to review, I skip over ones
> that already show an assigned reviewer, because I don't want to
> step on that person's toes.  But it seems very common to put
> one's name down for review without any immediate intention of
> doing work.  Or to do a review and wander off, leaving the patch
> apparently being tended to but not really.  (And I confess I'm
> occasionally guilty of both things myself.)
>
> I think it'd be great if we could separate "I'm actively reviewing
> this" from "I'm interested in this".  As a bonus, adding yourself
> to the "interested" list would be a fine proxy for the thumbs-up
> or star markers mentioned upthread.
>
> If those were separate columns, we could implement some sort of
> aging scheme whereby somebody who'd not commented for (say)
> a week or two would get quasi-automatically moved from the "active
> reviewer" column to the "interested" column, whereupon it wouldn't
> be impolite for someone else to sign up for active review.

I really like the idea of an "interested" column of some sort. I think
having some idea of what patches have interest is independently
valuable and helps us distinguish patches that no committer was
interested enough to work on and patches that no one thinks is a good
idea at all.

As for having multiple categories of reviewer, it's almost like we
need someone to take responsibility for shifting the patch to the next
state -- where the next state isn't necessarily "committed". We tend
to wait and assign a committer if the patch is actually committable
and will get committed. Lots of people review a patch without claiming
that were the author to address all of the feedback, the patch would
be committable. It might be helpful if someone could sign up to
shepherd the patch to its next state -- regardless of what that state
is.

- Melanie

On Thu, May 16, 2024 at 5:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Right, so what can we do about that?  Does Needs Review state need to
> be subdivided, and if so how?

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

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

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.

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

On 17.05.24 00:13, Tom Lane wrote:
>> 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.
> 
> Hmm, "forgotten by author" seems to generally turn into "this has been
> in WOA state a long time".  Not sure we have a problem representing
> that, only with a process for eventually retiring such entries.
> Your other three examples all sound like "needs senior developer
> attention", which could be a helpful state that's distinct from "ready
> for committer".  It's definitely not the same as "Unclear".

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

On 17.05.24 04:26, Robert Haas wrote:
> I do*emphatically*  think we need a parking lot.

People seem to like this idea; I'm not quite sure I follow it.  If you 
just want the automated patch testing, you can do that on your own by 
setting up github/cirrus for your own account.  If you keep emailing the 
public your patches just because you don't want to set up your private 
testing tooling, that seems a bit abusive?

Are there other reasons why developers might want their patches 
registered in a parking lot?

We also need to consider that the cfbot/cirrus resources are limited. 
Whatever changes we make, we should make sure that they are prioritized 
well.

On 17/05/2024 08:05, Peter Eisentraut wrote:
> On 17.05.24 04:26, Robert Haas wrote:
>> I do*emphatically*  think we need a parking lot.
> 
> People seem to like this idea; I'm not quite sure I follow it.  If you
> just want the automated patch testing, you can do that on your own by
> setting up github/cirrus for your own account.  If you keep emailing the
> public your patches just because you don't want to set up your private
> testing tooling, that seems a bit abusive?

Agreed. Also, if you do want to park a patch in the commitfest, setting 
it to "Waiting on Author" is effectively that.

I used to add patches to the commitfest to run CFBot on them, but some 
time back I bit the bullet and set up github/cirrus to run on my own 
github repository. I highly recommend that. It only takes a few clicks, 
and the user experience is much better: push a branch to my own github 
repository, and cirrus CI runs automatically.

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

On 17/05/2024 05:26, Robert Haas wrote:
> 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.

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.

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

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)

On Fri, 17 May 2024 at 06:58, Peter Eisentraut <peter@eisentraut.org> 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 <hlinnaka@iki.fi> wrote:
> 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.

+many on the free form text box

> On 17 May 2024, at 09:32, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

> On the mailing list, notes like that are both noisy and easily lost in the threads. But as a little free-form text
boxon 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,
wede-value the mailing list archives. I'm not too worried about it, the idea is that the summary box just summarizes
what'salready been said on the mailing list, or is transient information like "I'll get to this tomorrow" that's not
interestingto 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

> On 17 May 2024, at 00:03, Peter Eisentraut <peter@eisentraut.org> 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

On Fri, 17 May 2024 at 10:47, Daniel Gustafsson <daniel@yesql.se> wrote:
> 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"?

This sounds great! Especially if  Going back from an archived thread
to the commitfest entry or git commit is currently very hard. If I'll
just be able to Ctrl+F on commitfest@postgresql.com that would be so
helpful. I think it would even be useful to have an email be sent
whenever a patch gets initially added to the commitfest, so that
there's a back link to and it's easy to mark yourself as reviewer.
Right now, I almost never take the time to do that because if I look
at my inbox, I have no clue what the interesting email thread is
called in the commitfest app.

On Fri, 17 May 2024 at 11:02, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Fri, 17 May 2024 at 10:47, Daniel Gustafsson <daniel@yesql.se> wrote:
> > 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"?
>
> This sounds great! Especially if

(oops pressed send too early)
**Especially if it contains the git commit hash**

> Going back from an archived thread
> to the commitfest entry or git commit is currently very hard. If I'll
> just be able to Ctrl+F on commitfest@postgresql.com that would be so
> helpful. I think it would even be useful to have an email be sent
> whenever a patch gets initially added to the commitfest, so that
> there's a back link to and it's easy to mark yourself as reviewer.
> Right now, I almost never take the time to do that because if I look
> at my inbox, I have no clue what the interesting email thread is
> called in the commitfest app.

> On 16 May 2024, at 23:30, Robert Haas <robertmhaas@gmail.com> wrote:
>

I think we just need 10x CFMs. Let’s have a CFM for each CF section. I’d happily take "Replication and Recovery” or
“SystemAdministration” for July. “Miscellaneous” or “Performance” look monstrous. 
I feel I can easily track ~20 threads (besides threads I’m interested in). But it’s too tedious to spread attention
among~200 items. 


Best regards, Andrey Borodin.

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

On Fri, May 17, 2024 at 2:33 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 17/05/2024 05:26, Robert Haas wrote:
> > 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.
>
> 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.
>
> > 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.

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

On 5/16/24 23:43, Peter Eisentraut wrote:
> On 16.05.24 23:06, Joe Conway wrote:
>> On 5/16/24 16:57, Jacob Champion wrote:
>>> On Thu, May 16, 2024 at 1:31 PM Joe Conway <mail@joeconway.com> wrote:
>>>> Maybe we should just make it a policy that *nothing* gets moved forward
>>>> from commitfest-to-commitfest and therefore the author needs to care
>>>> enough to register for the next one?
>>>
>>> I think that's going to severely disadvantage anyone who doesn't do
>>> this as their day job. Maybe I'm bristling a bit too much at the
>>> wording, but not having time to shepherd a patch is not the same as
>>> not caring.
>>
>> Maybe the word "care" was a poor choice, but forcing authors to think
>> about and decide if they have the "time to shepherd a patch" for the
>> *next CF* is exactly the point. If they don't, why clutter the CF with
>> it.
> 
> Objectively, I think this could be quite effective.  You need to prove
> your continued interest in your project by pressing a button every two
> months.
> 
> But there is a high risk that this will double the annoyance for
> contributors whose patches aren't getting reviews.  Now, not only are
> you being ignored, but you need to prove that you're still there every
> two months.
> 

Yeah, I 100% agree with this. If a patch bitrots and no one cares enough
to rebase it once in a while, then sure - it's probably fine to mark it
RwF. But forcing all contributors to do a dance every 2 months just to
have a chance someone might take a look, seems ... not great.

I try to see this from the contributors' PoV, and with this process I'm
sure I'd start questioning if I even want to submit patches.

That is not to say we don't have a problem with patches that just move
to the next CF, and that we don't need to do something about that ...

Incidentally, I've been preparing some git+CF stats because of a talk
I'm expected to do, and it's very obvious the number of committed (and
rejected) CF entries is more or very stable over time, while the number
of patches that move to the next CF just snowballs.

My impression is a lot of these contributions/patches just never get the
review & attention that would allow them to move forward. Sure, some do
bitrot and/or get abandoned, and let's RwF those. But forcing everyone
to re-register the patches over and over seems like "reject by default".
I'd expect a lot of people to stop bothering and give up, and in a way
that would "solve" the bottleneck. But I'm not sure it's the solution
we'd like ...

It does seem to me a part of the solution needs to be helping to get
those patches reviewed. I don't know how to do that, but perhaps there's
a way to encourage people to review more stuff, or review stuff from a
wider range of contributors. Say by treating reviews more like proper
contributions.

Long time ago there was a "rule" that people submitting patches are
expected to do reviews. Perhaps we should be more strict this.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 17, 2024 at 1:05 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> On 17.05.24 04:26, Robert Haas wrote:
> > I do*emphatically*  think we need a parking lot.
>
> People seem to like this idea; I'm not quite sure I follow it.  If you
> just want the automated patch testing, you can do that on your own by
> setting up github/cirrus for your own account.  If you keep emailing the
> public your patches just because you don't want to set up your private
> testing tooling, that seems a bit abusive?
>
> Are there other reasons why developers might want their patches
> registered in a parking lot?

It's easier to say what's happening than it is to say why it's
happening. The CF contains a lot of stuff that appears to just be
parked there, so evidently reasons exist.

But if we are to guess what those reasons might be, Tom has already
admitted he does that for CI, and I do the same, so probably other
people also do it. I also suspect that some people are essentially
using the CF app as a personal todo list. By sticking patches in there
that they intend to commit next cycle, they both (1) feel virtuous,
because they give at least the appearance of following the community
process and inviting review before they commit and (2) avoid losing
track of the stuff they plan to commit.

There may be other reasons, too.

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

> On 17 May 2024, at 13:13, Robert Haas <robertmhaas@gmail.com> wrote:

> But if we are to guess what those reasons might be, Tom has already
> admitted he does that for CI, and I do the same, so probably other
> people also do it. I also suspect that some people are essentially
> using the CF app as a personal todo list. By sticking patches in there
> that they intend to commit next cycle, they both (1) feel virtuous,
> because they give at least the appearance of following the community
> process and inviting review before they commit and (2) avoid losing
> track of the stuff they plan to commit.
>
> There may be other reasons, too.

I think there is one more which is important: 3) Giving visibility into "this
is what I intend to commit".  Few can follow -hackers to the level where they
can have an overview of ongoing and/or finished work which will go in.  The CF
app does however provide that overview.  This is essentially the TODO list
aspect, but sharing one's TODO isn't all bad, especially for maintainers.

--
Daniel Gustafsson

On Fri, May 17, 2024 at 7:11 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
> Yeah, I 100% agree with this. If a patch bitrots and no one cares enough
> to rebase it once in a while, then sure - it's probably fine to mark it
> RwF. But forcing all contributors to do a dance every 2 months just to
> have a chance someone might take a look, seems ... not great.

I don't think that clicking on a link that someone sends you that asks
"hey, is this ready to be reviewed' qualifies as a dance.

I'm open to other proposals. But I think that if the proposal is
essentially "Hey, let's have the CFMs try harder to do the thing that
we've already been asking them to try to do for the last N years,"
then we might as well just not bother. It's obviously not working, and
it's not going to start working because we repeat the same things
about bouncing patches more aggressively. I just spent 2 days on it
and moved a handful of entries forward. To make a single pass over the
whole CommitFest at the rate I was going would take at least two
weeks, maybe three. And I am a highly experienced committer and
CommitFest manager with good facility in English and a lot of
experience arguing on this mailing list. I'm in the best possible
position to be able to do this well and efficiently and I can't. So
where are we going to find the people who can?

I think Andrey Borodin's nearby suggestion of having a separate CfM
for each section of the CommitFest does a good job revealing just how
bad the current situation is. I agree with him: that would actually
work. Asking somebody, for a one-month period, to be responsible for
shepherding one-tenth or one-twentieth of the entries in the
CommitFest would be a reasonable amount of work for somebody. But we
will not find 10 or 20 highly motivated, well-qualified volunteers
every other month to do that work; it's a struggle to find one or two
highly motivated, well-qualified CommitFest managers, let alone ten or
twenty. So I think the right interpretation of his comment is that
managing the CommitFest has become about an order of magnitude more
difficult than what it needs to be for the task to be done well.

> It does seem to me a part of the solution needs to be helping to get
> those patches reviewed. I don't know how to do that, but perhaps there's
> a way to encourage people to review more stuff, or review stuff from a
> wider range of contributors. Say by treating reviews more like proper
> contributions.

Well, I know that what would encourage *me* to do that is if I could
find the patches that fall into this category easily. I'm still not
going to spend all of my time on it, but when I do have time to spend
on it, I'd rather spend it on stuff that matters than on trying to
drain the CommitFest swamp. And right now every time I think "oh, I
should spend some time reviewing other people's patches," that time
promptly evaporates trying to find the patches that actually need
attention. I rarely get beyond the "Bug fixes" section of the
CommitFest application before I've used up all my available time, not
least because some people have figured out that labelling something
they don't like as a Bug fix gets it closer to the top of the CF list,
which is alphabetical by section.

> Long time ago there was a "rule" that people submitting patches are
> expected to do reviews. Perhaps we should be more strict this.

This sounds like it's just generating more manual work to add to a
system that's already suffering from needing too much manual work. Who
would keep track of how many reviews each person is doing, and how
many patches they're submitting, and whether those reviews were
actually any good, and what would they do about it?

One patch that comes to mind here is Thomas Munro's patch for
"unaccent: understand ancient Greek "oxia" and other codepoints merged
by Unicode". Somebody submitted a bug report and Thomas wrote a patch
and added it to the CommitFest. But there are open questions that need
to be researched, and this isn't really a priority for Thomas: he was
just trying to be nice and put somebody's bug report on a track to
resolution. Now, Thomas has many patches in the CommitFest, so if you
ask "does he review as much stuff as he has signed up to be reviewed,"
he clearly doesn't. Let's reject all of his patches, including this
one! And if on this specific patch you ask whether the author is
staying on top of it, he clearly isn't, so let's reject this one a
second time, just for that. Now, what have we accomplished by doing
all of that?

Not a whole lot, in general, because Thomas is a committer, so he can
still commit those patches if he wants, barring objections. However,
we have succeeded in kicking them out of our CI system, so if he does
commit them, they'll be more likely to break the buildfarm. And in the
case of this specific patch, what we've done is punish Thomas for
trying to help out somebody who submitted a bug report, and at the
same time, made the patch he submitted less visible to anyone who
might want to help with it.

Wahoo!

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

On 5/16/24 22:26, Robert Haas wrote:
> 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.

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

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

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
> once-upon-a-time-I-wrote-a-patch-Fest.

+1

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

On Fri, 17 May 2024 at 13:39, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, May 17, 2024 at 7:11 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
> > Yeah, I 100% agree with this. If a patch bitrots and no one cares enough
> > to rebase it once in a while, then sure - it's probably fine to mark it
> > RwF. But forcing all contributors to do a dance every 2 months just to
> > have a chance someone might take a look, seems ... not great.
>
> I don't think that clicking on a link that someone sends you that asks
> "hey, is this ready to be reviewed' qualifies as a dance.

If there's been any useful response to the patch since the last time
you pressed this button, then it might be okay. But it's definitely
not uncommon for items on the commitfest app to get no actual response
at all for half a year, i.e. multiple commits fests (except for the
odd request for a rebase that an author does within a week). I'd most
certainly get very annoyed if for those patches where it already seems
as if I'm screaming into the void I'd also be required to press a
button every two months, for it to even have a chance at receiving a
response.

> So I think the right interpretation of his comment is that
> managing the CommitFest has become about an order of magnitude more
> difficult than what it needs to be for the task to be done well.

+1

> > Long time ago there was a "rule" that people submitting patches are
> > expected to do reviews. Perhaps we should be more strict this.
>
> This sounds like it's just generating more manual work to add to a
> system that's already suffering from needing too much manual work. Who
> would keep track of how many reviews each person is doing, and how
> many patches they're submitting, and whether those reviews were
> actually any good, and what would they do about it?

+1

On Fri, 17 May 2024 at 14:19, Joe Conway <mail@joeconway.com> wrote:
>
> On 5/16/24 22:26, Robert Haas wrote:
> > 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.
>
> 100% agree. This is in line with what I suggested on an adjacent part of
> the thread.

Such a proposal would basically mean that no-one that cares about
their patches getting reviews can go on holiday and leave work behind
during the week before a commit fest. That seems quite undesirable to
me.

On 5/17/24 08:31, Jelte Fennema-Nio wrote:
> On Fri, 17 May 2024 at 14:19, Joe Conway <mail@joeconway.com> wrote:
>>
>> On 5/16/24 22:26, Robert Haas wrote:
>> > 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.
>>
>> 100% agree. This is in line with what I suggested on an adjacent part of
>> the thread.
> 
> Such a proposal would basically mean that no-one that cares about
> their patches getting reviews can go on holiday and leave work behind
> during the week before a commit fest. That seems quite undesirable to
> me.

Well, I'm sure I'll get flamed for this suggestion, be here goes anyway...

I wrote:
> 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.

A solution to both of these issues (yours and mine) would be to allow 
things to be added *during* the CF month. What is the point of having a 
"freeze" before every CF anyway? Especially if they start out clean. If 
something is ready for review on day 8 of the CF, why not let it be 
added for review?


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

On 17.05.24 13:36, Daniel Gustafsson wrote:
>> On 17 May 2024, at 13:13, Robert Haas <robertmhaas@gmail.com> wrote:
> 
>> But if we are to guess what those reasons might be, Tom has already
>> admitted he does that for CI, and I do the same, so probably other
>> people also do it. I also suspect that some people are essentially
>> using the CF app as a personal todo list. By sticking patches in there
>> that they intend to commit next cycle, they both (1) feel virtuous,
>> because they give at least the appearance of following the community
>> process and inviting review before they commit and (2) avoid losing
>> track of the stuff they plan to commit.
>>
>> There may be other reasons, too.
> 
> I think there is one more which is important: 3) Giving visibility into "this
> is what I intend to commit".  Few can follow -hackers to the level where they
> can have an overview of ongoing and/or finished work which will go in.  The CF
> app does however provide that overview.  This is essentially the TODO list
> aspect, but sharing one's TODO isn't all bad, especially for maintainers.

Ok, but these cases shouldn't use a separate "parking lot".  They should 
use the same statuses and flow diagram as the rest.  (Maybe with more 
dotted lines, sure.)

> On 17 May 2024, at 16:39, Robert Haas <robertmhaas@gmail.com> wrote:
>
> I think Andrey Borodin's nearby suggestion of having a separate CfM
> for each section of the CommitFest does a good job revealing just how
> bad the current situation is. I agree with him: that would actually
> work. Asking somebody, for a one-month period, to be responsible for
> shepherding one-tenth or one-twentieth of the entries in the
> CommitFest would be a reasonable amount of work for somebody. But we
> will not find 10 or 20 highly motivated, well-qualified volunteers
> every other month to do that work;

Why do you think so? Let’s just try to find more CFMs for July.
When I felt that I’m overwhelmed, I asked for help and Alexander Alekseev promptly agreed. That helped a lot.
If I was in that position again, I would just ask 10 times on a 1st day :)

> it's a struggle to find one or two
> highly motivated, well-qualified CommitFest managers, let alone ten or
> twenty.

Because we are looking for one person to do a job for 10.

> So I think the right interpretation of his comment is that
> managing the CommitFest has become about an order of magnitude more
> difficult than what it needs to be for the task to be done well.

Let’s scale the process. Reduce responsibility area of a CFM, define it clearer.
And maybe even explicitly ask CFM to summarize patch status of each entry at least once a CF.


Can I do a small poll among those who is on this thread? Would you volunteer to summarize a status of 20 patches in
July’sCF? 5 each week or so. One per day. 


Best regards, Andrey Borodin.

I wrote:

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.

A solution to both of these issues (yours and mine) would be to allow things to be added *during* the CF month. What is the point of having a "freeze" before every CF anyway? Especially if they start out clean. If something is ready for review on day 8 of the CF, why not let it be added for review?

On 17.05.24 09:32, Heikki Linnakangas wrote:
> 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.

We already have the annotations feature, which is kind of this.

On Fri, May 17, 2024 at 8:31 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> Such a proposal would basically mean that no-one that cares about
> their patches getting reviews can go on holiday and leave work behind
> during the week before a commit fest. That seems quite undesirable to
> me.

Well, then we make it ten days instead of seven, or give a few days
grace after the CF starts to play catchup, or allow the CfM to make
exceptions.

To be fair, I'm not sure that forcing people to do something like this
is going to solve our problem. I'm very open to other ideas. But one
idea that I'm not open to is to just keep doing what we're doing. It
clearly and obviously does not work.

I just tried scrolling through the CommitFest to a more or less random
spot by flicking the mouse up and down, and then clicked on whatever
ended up in the middle of my screen. I did this four times. Two of
those landed on patches that had extremely long discussion threads
already. One hit a patch from a non-committer that hasn't been
reviewed and needs to be. And the fourth hit a patch from a committer
which maybe could benefit from review but I can already guess that the
patch works fine and unless somebody can find some architectural
downside to the approach taken, there's not really a whole lot to talk
about.

I don't entirely know how to think about that result, but it seems
pretty clear that the unreviewed non-committer patch ought to get
priority, especially if we're talking about the possibility of
non-committers or even junior committers doing drive-by reviews. The
high-quality committer patch might be worth a comment from me, pro or
con or whatever, but it's probably not a great use of time for a more
casual contributor: they probably aren't going to find too much wrong
with it. And the threads with extremely long threads already, well, I
don't know if there's something useful that can be done with those
threads or not, but those patches certainly haven't been ignored.

I'm not sure that any of these should be evicted from the CommitFest,
but we need to think about how to impose some structure on the chaos.
Just classifying all four of those entries as either "Needs Review" or
"Waiting on Author" is pretty useless; then they all look the same,
and they're not. And please don't suggest adding a bunch more status
values that the CfM has to manually police as the solution. We need to
find some way to create a system that does the right thing more often
by default.

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

On Fri, May 17, 2024 at 9:02 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> We already have the annotations feature, which is kind of this.

I didn't realize we had that feature. When was it added, and how is it
supposed to be used?

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

On 17.05.24 14:42, Joe Conway wrote:
>> 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.
> 
> A solution to both of these issues (yours and mine) would be to allow 
> things to be added *during* the CF month. What is the point of having a 
> "freeze" before every CF anyway? Especially if they start out clean. If 
> something is ready for review on day 8 of the CF, why not let it be 
> added for review?

Maybe this all indicates that the idea of bimonthly commitfests has run 
its course.  The original idea might have been, if we have like 50 
patches, we can process all of them within a month.  We're clearly not 
doing that anymore.  How would the development process look like if we 
just had one commitfest per dev cycle that runs from July 1st to March 31st?

On 5/17/24 14:51, Andrey M. Borodin wrote:
> 
> 
>> On 17 May 2024, at 16:39, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> I think Andrey Borodin's nearby suggestion of having a separate CfM
>> for each section of the CommitFest does a good job revealing just how
>> bad the current situation is. I agree with him: that would actually
>> work. Asking somebody, for a one-month period, to be responsible for
>> shepherding one-tenth or one-twentieth of the entries in the
>> CommitFest would be a reasonable amount of work for somebody. But we
>> will not find 10 or 20 highly motivated, well-qualified volunteers
>> every other month to do that work;
> 
> Why do you think so? Let’s just try to find more CFMs for July.
> When I felt that I’m overwhelmed, I asked for help and Alexander Alekseev promptly agreed. That helped a lot.
> If I was in that position again, I would just ask 10 times on a 1st day :)
> 
>> it's a struggle to find one or two
>> highly motivated, well-qualified CommitFest managers, let alone ten or
>> twenty.
> 
> Because we are looking for one person to do a job for 10.
> 

Yes. It's probably easier to find more CF managers doing much less work.

>> So I think the right interpretation of his comment is that
>> managing the CommitFest has become about an order of magnitude more
>> difficult than what it needs to be for the task to be done well.
> 
> Let’s scale the process. Reduce responsibility area of a CFM, define it clearer.
> And maybe even explicitly ask CFM to summarize patch status of each entry at least once a CF.
> 

Should it even be up to the CFM to write the summary, or should he/she
be able to request an update from the patch author? Of at least have the
choice to do so.

I think we'll always struggle with the massive threads, because it's
really difficult to find the right balance between brevity and including
all the relevant details. Or rather impossible. I did try writing such
summaries for a couple of my long-running patches, and while it might
have helped, the challenge was to also explain why stuff *not* done in
some alternative way, which is one of the things usually discussed. But
the summary gets very long, because there are many alternatives.

> 
> Can I do a small poll among those who is on this thread? Would you
volunteer to summarize a status of 20 patches in July’s CF? 5 each week
or so. One per day.
> 

Not sure. For many patches it'll be trivial. And for a bunch it'll be
very very time-consuming.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On 17 May 2024, at 15:05, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, May 17, 2024 at 9:02 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> We already have the annotations feature, which is kind of this.
>
> I didn't realize we had that feature. When was it added, and how is it
> supposed to be used?

A while back.

commit 27cba025a501c9dbcfb08da0c4db95dc6111d647
Author: Magnus Hagander <magnus@hagander.net>
Date:   Sat Feb 14 13:07:48 2015 +0100

    Implement simple message annotations

    This feature makes it possible to "pull in" a message in a thread and highlight
    it with an annotation (free text format). This will list the message in a table
    along with the annotation and who made it.

    Annotations have to be attached to a specific message - for a "generic" one it
    makes sense to attach it to the latest message available, as that will put it
    at the correct place in time.

Magnus' commitmessage explains it well.  The way I've used it (albeit
infrequently) is to point to a specific mail in the thread where a significant
change was proposed, like the patch changhing direction or something similar.

--
Daniel Gustafsson

On 5/17/24 09:08, Peter Eisentraut wrote:
> On 17.05.24 14:42, Joe Conway wrote:
>>> 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.
>> 
>> A solution to both of these issues (yours and mine) would be to allow 
>> things to be added *during* the CF month. What is the point of having a 
>> "freeze" before every CF anyway? Especially if they start out clean. If 
>> something is ready for review on day 8 of the CF, why not let it be 
>> added for review?
> 
> Maybe this all indicates that the idea of bimonthly commitfests has run
> its course.  The original idea might have been, if we have like 50
> patches, we can process all of them within a month.  We're clearly not
> doing that anymore.  How would the development process look like if we
> just had one commitfest per dev cycle that runs from July 1st to March 31st?

What's old is new again? ;-)

I agree with you. Before commitfests were a thing, we had no structure 
at all as I recall. The dates for the dev cycle were more fluid as I 
recall, and we had no CF app to track things. Maybe retaining the 
structure but going back to the continuous development would be just the 
thing, with the CF app tracking just the currently (as of this 
week/month/???) active stuff.


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

On Fri, May 17, 2024 at 9:54 AM Joe Conway <mail@joeconway.com> wrote:
> I agree with you. Before commitfests were a thing, we had no structure
> at all as I recall. The dates for the dev cycle were more fluid as I
> recall, and we had no CF app to track things. Maybe retaining the
> structure but going back to the continuous development would be just the
> thing, with the CF app tracking just the currently (as of this
> week/month/???) active stuff.

The main thing that we'd gain from that is to avoid all the work of
pushing lots of things forward to the next CommitFest at the end of
every CommitFest. While I agree that we need to find a better way to
handle that, I don't think it's really the biggest problem. The core
problems here are (1) keeping stuff out of CommitFests that don't
belong there and (2) labelling stuff that does belong in the
CommitFest in useful ways. We should shape the solution around those
problems. Maybe that will solve this problem along the way, but if it
doesn't, that's easy enough to fix afterward.

Like, we could also just have a button that says "move everything
that's left to the next CommitFest". That, too, would avoid the manual
work that this would avoid. But it wouldn't solve any other problems,
so it's not really worth much consideration.

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

On Fri, May 17, 2024 at 9:09 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> How would the development process look like if we
> just had one commitfest per dev cycle that runs from July 1st to March 31st?

Exactly the same?

--
Peter Geoghegan

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 17, 2024 at 9:54 AM Joe Conway <mail@joeconway.com> wrote:
>> I agree with you. Before commitfests were a thing, we had no structure
>> at all as I recall. The dates for the dev cycle were more fluid as I
>> recall, and we had no CF app to track things. Maybe retaining the
>> structure but going back to the continuous development would be just the
>> thing, with the CF app tracking just the currently (as of this
>> week/month/???) active stuff.

> The main thing that we'd gain from that is to avoid all the work of
> pushing lots of things forward to the next CommitFest at the end of
> every CommitFest.

To my mind, the point of the time-boxed commitfests is to provide
a structure wherein people will (hopefully) pay some actual attention
to other peoples' patches.  Conversely, the fact that we don't have
one running all the time gives committers some defined intervals
where they can work on their own stuff without feeling guilty that
they're not handling other people's patches.

If we go back to the old its-development-mode-all-the-time approach,
what is likely to happen is that the commit rate for not-your-own-
patches goes to zero, because it's always possible to rationalize
your own stuff as being more important.

> Like, we could also just have a button that says "move everything
> that's left to the next CommitFest".

Clearly, CFMs would appreciate some more tooling to make that sort
of thing easier.  Perhaps we omitted it in the original CF app
coding because we expected the end-of-CF backlog to be minimal,
but it's now obvious that it generally isn't.

BTW, I was reminded while trawling old email yesterday that
we used to freeze the content of a CF at its start and then
hold the CF open until the backlog actually went to zero,
which resulted in multi-month death-march CFs and no clarity
at all as to release timing.  Let's not go back to that.
But the CF app was probably built with that model in mind.

            regards, tom lane

On Fri, May 17, 2024 at 10:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> To my mind, the point of the time-boxed commitfests is to provide
> a structure wherein people will (hopefully) pay some actual attention
> to other peoples' patches.  Conversely, the fact that we don't have
> one running all the time gives committers some defined intervals
> where they can work on their own stuff without feeling guilty that
> they're not handling other people's patches.
>
> If we go back to the old its-development-mode-all-the-time approach,
> what is likely to happen is that the commit rate for not-your-own-
> patches goes to zero, because it's always possible to rationalize
> your own stuff as being more important.

We already have gone back to that model. We just haven't admitted it
yet. And we're never going to get out of it until we find a way to get
the contents of the CommitFest application down to a more reasonable
size and level of complexity. There's just no way everyone's up for
that level of pain. I'm not sure not up for that level of pain.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 17, 2024 at 10:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If we go back to the old its-development-mode-all-the-time approach,
>> what is likely to happen is that the commit rate for not-your-own-
>> patches goes to zero, because it's always possible to rationalize
>> your own stuff as being more important.

> We already have gone back to that model. We just haven't admitted it
> yet. And we're never going to get out of it until we find a way to get
> the contents of the CommitFest application down to a more reasonable
> size and level of complexity. There's just no way everyone's up for
> that level of pain. I'm not sure not up for that level of pain.

Yeah, we clearly need to get the patch list to a point of
manageability, but I don't agree that abandoning time-boxed CFs
will improve anything.

            regards, tom lane

On Fri, May 17, 2024 at 7:11 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> On 5/16/24 23:43, Peter Eisentraut wrote:
> > On 16.05.24 23:06, Joe Conway wrote:
> >> On 5/16/24 16:57, Jacob Champion wrote:
> >>> On Thu, May 16, 2024 at 1:31 PM Joe Conway <mail@joeconway.com> wrote:
> >>>> Maybe we should just make it a policy that *nothing* gets moved forward
> >>>> from commitfest-to-commitfest and therefore the author needs to care
> >>>> enough to register for the next one?
> >>>
> >>> I think that's going to severely disadvantage anyone who doesn't do
> >>> this as their day job. Maybe I'm bristling a bit too much at the
> >>> wording, but not having time to shepherd a patch is not the same as
> >>> not caring.
> >>
> >> Maybe the word "care" was a poor choice, but forcing authors to think
> >> about and decide if they have the "time to shepherd a patch" for the
> >> *next CF* is exactly the point. If they don't, why clutter the CF with
> >> it.
> >
> > Objectively, I think this could be quite effective.  You need to prove
> > your continued interest in your project by pressing a button every two
> > months.
> >
> > But there is a high risk that this will double the annoyance for
> > contributors whose patches aren't getting reviews.  Now, not only are
> > you being ignored, but you need to prove that you're still there every
> > two months.
> >
>
> Yeah, I 100% agree with this. If a patch bitrots and no one cares enough
> to rebase it once in a while, then sure - it's probably fine to mark it
> RwF. But forcing all contributors to do a dance every 2 months just to
> have a chance someone might take a look, seems ... not great.
>
> I try to see this from the contributors' PoV, and with this process I'm
> sure I'd start questioning if I even want to submit patches.

Agreed.

> That is not to say we don't have a problem with patches that just move
> to the next CF, and that we don't need to do something about that ...
>
> Incidentally, I've been preparing some git+CF stats because of a talk
> I'm expected to do, and it's very obvious the number of committed (and
> rejected) CF entries is more or very stable over time, while the number
> of patches that move to the next CF just snowballs.
>
> My impression is a lot of these contributions/patches just never get the
> review & attention that would allow them to move forward. Sure, some do
> bitrot and/or get abandoned, and let's RwF those. But forcing everyone
> to re-register the patches over and over seems like "reject by default".
> I'd expect a lot of people to stop bothering and give up, and in a way
> that would "solve" the bottleneck. But I'm not sure it's the solution
> we'd like ...

I don't think we should reject by default. It is discouraging and it
is already hard enough as it is to contribute.

> It does seem to me a part of the solution needs to be helping to get
> those patches reviewed. I don't know how to do that, but perhaps there's
> a way to encourage people to review more stuff, or review stuff from a
> wider range of contributors. Say by treating reviews more like proper
> contributions.

One reason I support the parking lot idea is for patches like the one
in [1]. EXPLAIN for parallel bitmap heap scan is just plain broken.
The patch in this commitfest entry is functionally 80% of the way
there. It just needs someone to do the rest of the work to make it
committable. I actually think it is unreasonable of us to expect the
original author to do this. I have had it on my list for weeks to get
back around to helping with this patch. However, I spent the better
part of my coding time in the last two weeks trying to reproduce and
fix a bug on stable branches that causes vacuum processes to
infinitely loop. Arguably that is a bigger problem. Because I knew
this EXPLAIN patch was slipping down my TODO list, I changed the patch
to "waiting on author", but I honestly don't think the original author
should have to do the rest of the work.

Should I spend more time on this patch reviewing it and moving it
forward? Yes. Maybe I'm just too slow at writing postgres code or I
have bad time management or I should spend  less time doing things
like figuring out how many lavalier mics we need in each room for
PGConf.dev. I don't know. But it is hard for me to figure out how to
do more review work and guarantee that this kind of thing won't
happen.

So, anyway, I'd argue that we need a parking lot for patches which we
all agree are important and have a path forward but need someone to do
the last 20-80% of the work. To avoid this being a dumping ground,
patches should _only_ be allowed in the parking lot if they have a
clear path forward. Patches which haven't gotten any interest don't go
there. Patches in which the author has clearly not addressed feedback
that is reasonable for them to address don't go there. These are
effectively community TODOs which we agree need to be done -- if only
someone had the time.

- Melanie

[1] https://commitfest.postgresql.org/48/4248/

On Fri, May 17, 2024 at 7:40 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, May 17, 2024 at 10:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > To my mind, the point of the time-boxed commitfests is to provide
> > a structure wherein people will (hopefully) pay some actual attention
> > to other peoples' patches.  Conversely, the fact that we don't have
> > one running all the time gives committers some defined intervals
> > where they can work on their own stuff without feeling guilty that
> > they're not handling other people's patches.
> >
> > If we go back to the old its-development-mode-all-the-time approach,
> > what is likely to happen is that the commit rate for not-your-own-
> > patches goes to zero, because it's always possible to rationalize
> > your own stuff as being more important.
>
> We already have gone back to that model. We just haven't admitted it
> yet.

I've worked on teams that used the short-timebox CF calendar to
organize community work, like Tom describes. That was a really
positive thing for us.

Maybe it feels different from the committer point of view, but I don't
think all of the community is operating on the long-timebox model, and
I really wouldn't want to see us lengthen the cycles to try to get
around the lack of review/organization that's being complained about.
(But maybe you're not arguing for that in the first place.)

--Jacob

Melanie Plageman <melanieplageman@gmail.com> writes:
> So, anyway, I'd argue that we need a parking lot for patches which we
> all agree are important and have a path forward but need someone to do
> the last 20-80% of the work. To avoid this being a dumping ground,
> patches should _only_ be allowed in the parking lot if they have a
> clear path forward. Patches which haven't gotten any interest don't go
> there. Patches in which the author has clearly not addressed feedback
> that is reasonable for them to address don't go there. These are
> effectively community TODOs which we agree need to be done -- if only
> someone had the time.

Hmm.  I was envisioning "parking lot" as meaning "this is on my
personal TODO list, and I'd like CI support for it, but I'm not
expecting anyone else to pay attention to it yet".  I think what
you are describing is valuable but different.  Maybe call it
"pending" or such?  Or invent a different name for the other thing.

            regards, tom lane

On Fri, May 17, 2024 at 11:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > We already have gone back to that model. We just haven't admitted it
> > yet. And we're never going to get out of it until we find a way to get
> > the contents of the CommitFest application down to a more reasonable
> > size and level of complexity. There's just no way everyone's up for
> > that level of pain. I'm not sure not up for that level of pain.
>
> Yeah, we clearly need to get the patch list to a point of
> manageability, but I don't agree that abandoning time-boxed CFs
> will improve anything.

I'm not sure. Suppose we plotted commits generally, or commits of
non-committer patches, or reviews on-list, vs. time. Would we see any
uptick in activity during CommitFests? Would it vary by committer? I
don't know. I bet the difference wouldn't be as much as Tom Lane would
like to see. Realistically, we can't manage how contributors spend
their time all that much, and trying to do so is largely tilting at
windmills.

To me, the value of time-based CommitFests is as a vehicle to ensure
freshness, or cleanup, or whatever word you want to do. If you just
make a list of things that need attention and keep incrementally
updating it, eventually for various reasons that list no longer
reflects your current list of priorities. At some point, you have to
throw it out and make a new list, or at least that's what always
happens to me. We've fallen into a system where the default treatment
of a patch is to be carried over to the next CommitFest and CfMs are
expected to exert effort to keep patches from getting that default
treatment when they shouldn't. But that does not scale. We need a
system where things drop off the list unless somebody makes an effort
to keep them on the list, and the easiest way to do that is to
periodically make a *fresh* list that *doesn't* just clone some
previous list.

I realize that many people here are (rightly!) concerned with
burdening patch authors with more steps that they have to follow. But
the current system is serving new patch authors very poorly. If they
get attention, it's much more likely to be because somebody saw their
email and wrote back than it is to be because somebody went through
the CommitFest and found their entry and was like "oh, I should review
this". Honestly, if we get to a situation where a patch author is sad
because they have to click a link every 2 months to say "yeah, I'm
still here, please review my patch," we've already lost the game. That
person isn't sad because we asked them to click a link. They're sad
it's already been N * 2 months and nothing has happened.

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

On Fri, May 17, 2024 at 11:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Melanie Plageman <melanieplageman@gmail.com> writes:
> > So, anyway, I'd argue that we need a parking lot for patches which we
> > all agree are important and have a path forward but need someone to do
> > the last 20-80% of the work. To avoid this being a dumping ground,
> > patches should _only_ be allowed in the parking lot if they have a
> > clear path forward. Patches which haven't gotten any interest don't go
> > there. Patches in which the author has clearly not addressed feedback
> > that is reasonable for them to address don't go there. These are
> > effectively community TODOs which we agree need to be done -- if only
> > someone had the time.
>
> Hmm.  I was envisioning "parking lot" as meaning "this is on my
> personal TODO list, and I'd like CI support for it, but I'm not
> expecting anyone else to pay attention to it yet".

+1.

> I think what
> you are describing is valuable but different.  Maybe call it
> "pending" or such?  Or invent a different name for the other thing.

Yeah, there should be someplace that we keep a list of things that are
thought to be important but we haven't gotten around to doing anything
about yet, but I think that's different from the parking lot CF.

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

On 5/17/24 11:58, Robert Haas wrote:
> I realize that many people here are (rightly!) concerned with
> burdening patch authors with more steps that they have to follow. But
> the current system is serving new patch authors very poorly. If they
> get attention, it's much more likely to be because somebody saw their
> email and wrote back than it is to be because somebody went through
> the CommitFest and found their entry and was like "oh, I should review
> this". Honestly, if we get to a situation where a patch author is sad
> because they have to click a link every 2 months to say "yeah, I'm
> still here, please review my patch," we've already lost the game. That
> person isn't sad because we asked them to click a link. They're sad
> it's already been N * 2 months and nothing has happened.

+many

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

On Fri, 17 May 2024 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote:
> If they
> get attention, it's much more likely to be because somebody saw their
> email and wrote back than it is to be because somebody went through
> the CommitFest and found their entry and was like "oh, I should review
> this".

I think this is an important insight. I used the commitfest app to
find patches to review when I was just starting out in postgres
development, since I didn't subscribe to all af pgsql-hackers yet. I
do subscribe nowadays, but now I rarely look at the commitfest app,
instead I skim the titles of emails that go into my "Postgres" folder
in my mailbox. Going from such an email to a commitfest entry is near
impossible (at least I don't know how to do this efficiently). I guess
I'm not the only one doing this.

> Honestly, if we get to a situation where a patch author is sad
> because they have to click a link every 2 months to say "yeah, I'm
> still here, please review my patch," we've already lost the game. That
> person isn't sad because we asked them to click a link. They're sad
> it's already been N * 2 months and nothing has happened.

Maybe it wouldn't be so bad for an author to click the 2 months
button, if it would actually give their patch some higher chance of
being reviewed by doing that. And given the previous insight, that
people don't look at the commitfest app that often, it might be good
if pressing this button would also bump the item in people's
mailboxes.

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Friday, May 17, 2024, Joe Conway <mail@joeconway.com> wrote:
>> A solution to both of these issues (yours and mine) would be to allow
>> things to be added *during* the CF month. What is the point of having a
>> "freeze" before every CF anyway? Especially if they start out clean. If
>> something is ready for review on day 8 of the CF, why not let it be added
>> for review?

> In conjunction with WIP removing this limitation on the bimonthlies makes
> sense to me.

I think that the original motivation for this was two-fold:

1. A notion of fairness, that you shouldn't get your patch reviewed
ahead of others that have been waiting (much?) longer.  I'm not sure
how much this is really worth.  In particular, even if we do delay
an incoming patch by one CF, it's still going to compete with the
older stuff in future CFs.  There's already a sort feature in the CF
dashboard whereby patches that have lingered for more CFs appear ahead
of patches that are newer, so maybe just ensuring that late-arriving
patches sort as "been around for 0 CFs" is sufficient.

2. As I mentioned a bit ago, the original idea was that we didn't exit
a CF until it was empty of un-handled patches, so obviously allowing
new patches to come in would mean we'd never get to empty.  That idea
didn't work and we don't think that way anymore.

So yeah, I'm okay with abandoning the must-submit-to-a-future-CF
restriction.

            regards, tom lane

On Fri, 2024-05-17 at 13:12 -0400, Greg Sabino Mullane wrote:
> > Long time ago there was a "rule" that people submitting patches are expected
> > to do reviews. Perhaps we should be more strict this.
>
> Big -1. How would we even be more strict about this? Public shaming? Withholding a commit?

I think it is a good rule.  I don't think that it shouldn't lead to putting
people on the pillory or kicking their patches, but I imagine that a committer
looking for somebody else's patch to work on could prefer patches by people
who are doing their share of reviews.

Yours,
Laurenz Albe

On Fri, May 17, 2024 at 3:51 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> On Fri, 2024-05-17 at 13:12 -0400, Greg Sabino Mullane wrote:
> > > Long time ago there was a "rule" that people submitting patches are expected
> > > to do reviews. Perhaps we should be more strict this.
> >
> > Big -1. How would we even be more strict about this? Public shaming? Withholding a commit?
>
> I think it is a good rule.  I don't think that it shouldn't lead to putting
> people on the pillory or kicking their patches, but I imagine that a committer
> looking for somebody else's patch to work on could prefer patches by people
> who are doing their share of reviews.

If you give me an automated way to find that out, I'll consider paying
some attention to it. However, in order to sort the list of patches
needing review by the amount of review done by the patch author, we'd
first need to have a list of patches needing review.

And right now we don't, or at least not in any usable way.
commitfest.postgresql.org is supposed to give us that, but it doesn't.

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

On 5/17/24 21:59, Robert Haas wrote:
> On Fri, May 17, 2024 at 3:51 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>> On Fri, 2024-05-17 at 13:12 -0400, Greg Sabino Mullane wrote:
>>>> Long time ago there was a "rule" that people submitting patches are expected
>>>> to do reviews. Perhaps we should be more strict this.
>>>
>>> Big -1. How would we even be more strict about this? Public shaming? Withholding a commit?
>>
>> I think it is a good rule.  I don't think that it shouldn't lead to putting
>> people on the pillory or kicking their patches, but I imagine that a committer
>> looking for somebody else's patch to work on could prefer patches by people
>> who are doing their share of reviews.
> 

Yeah, I don't have any particular idea how should the rule be "enforced"
and I certainly did not imagine public shaming or anything like that. My
thoughts were more about reminding people the reviews are part of the
deal, that's it ... maybe "more strict" was not quite what I meant.

> If you give me an automated way to find that out, I'll consider paying
> some attention to it. However, in order to sort the list of patches
> needing review by the amount of review done by the patch author, we'd
> first need to have a list of patches needing review.
> 
> And right now we don't, or at least not in any usable way.
> commitfest.postgresql.org is supposed to give us that, but it doesn't.
> 

It'd certainly help to know which patches to consider for review, but I
guess I'd still look at patches from people doing more reviews first,
even if I had to find out in what shape the patch is.

I'm far more skeptical about "automated way" to track this, though. I'm
not sure it's quite possible - reviews can have a lot of very different
forms, and deciding what is or is not a review is pretty subjective. So
it's not clear how would we quantify that. Not to mention I'm sure we'd
promptly find ways to game that.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 17, 2024 at 3:51 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>> I think it is a good rule.  I don't think that it shouldn't lead to putting
>> people on the pillory or kicking their patches, but I imagine that a committer
>> looking for somebody else's patch to work on could prefer patches by people
>> who are doing their share of reviews.

> If you give me an automated way to find that out, I'll consider paying
> some attention to it.

Yeah, I can't imagine that any committer (or reviewer, really) is
doing any such thing, because it would take far too much effort to
figure out how much work anyone else is doing.  I see CFMs reminding
everybody that this rule exists, but I don't think they ever try to
check it either.  It's pretty much the honor system, and I'm sure
some folk ignore it.

            regards, tom lane

On Fri, May 17, 2024 at 11:44:40AM -0400, Melanie Plageman wrote:
> So, anyway, I'd argue that we need a parking lot for patches which we
> all agree are important and have a path forward but need someone to do
> the last 20-80% of the work. To avoid this being a dumping ground,
> patches should _only_ be allowed in the parking lot if they have a
> clear path forward. Patches which haven't gotten any interest don't go
> there. Patches in which the author has clearly not addressed feedback
> that is reasonable for them to address don't go there. These are
> effectively community TODOs which we agree need to be done -- if only
> someone had the time.

When I am looking to commit something, I have to consider:

*  do we want the change
*  are there concerns
*  are the docs good
*  does it need tests
*  is it only a proof-of-concept

When people review commit fest entries, they have to figure out what is
holding the patch back from being complete, so they have to read the
thread from the beginning.  Should there be a clearer way in the commit
fest app to specify what is missing?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

> On Thu, May 16, 2024 at 02:30:03PM -0400, Robert Haas wrote:
>
> 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.

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?

On 5/19/24 05:37, Dmitry Dolgov wrote:
> * 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.


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

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

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> 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?

Things *have* changed, if you take the long view.  We didn't have
commitfests at all until around 2007, and we've changed the ground
rules for them a couple of times since then.  We didn't have the CF
app at all until, well, I don't recall when, but the first few CFs
were managed by keeping patch lists on a wiki page.  It's not that
people are unwilling to change this stuff, but that it's hard to
identify what will make things better.

IMV one really fundamental problem is the same as it's been for a
couple of decades: too many patch submissions, too few committers.
We can't fix that by just appointing a ton more committers, at least
not if we want to keep the project's quality up.  We have to grow
qualified committers.  IIRC, one of the main reasons for instituting
the commitfests at all was the hope that if we got more people to
spend time reading the code and reviewing patches, some of them would
learn enough to become good committers.  I think that's worked, again
taking a long view.  I just did some quick statistics on the commit
history, and I see that we were hovering at somewhere around ten
active committers from 1999 to 2012, but since then it's slowly crept
up to about two dozen today.  (I'm counting "active" as "at least 10
commits per year", which is an arbitrary cutoff --- feel free to slice
the data for yourself.)  Meanwhile the number of submissions has also
grown, so I'm not sure how much better the load ratio is.

My point here is not that things are great, but just that we are
indeed improving, and I hope we can continue to.  Let's not be
defeatist about it.

I think this thread has already identified a few things we have
consensus to improve in the CF app, and I hope somebody goes off
and makes those happen (I lack the web skillz to help myself).
However, the app itself is just a tool; what counts more is our
process around it.  I have a couple of thoughts about that:

* Patches that sit in the queue a long time tend to be ones that lack
consensus, either about the goal or the details of how to achieve it.
Sometimes "lacks consensus" really means "nobody but the author thinks
this is a good idea, but we haven't mustered the will to say no".
But I think it's more usually the case that there are plausible
competing opinions about what the patch should do or how it should
do it.  How can we resolve such differences and get something done?

* Another reason for things sitting a long time is that they're too
big to review without an unreasonable amount of effort.  We should
encourage authors to break large patches into smaller stepwise
refinements.  It may seem like that will result in taking forever
to reach the end goal, but dropping a huge patchset on the community
isn't going to give speedy results either.

* Before starting this thread, Robert did a lot of very valuable
review of some individual patches.  I think what prompted him to
start the thread was the realization that he'd only made a small
dent in the problem.  Maybe we could divide and conquer: get a
dozen-or-so senior contributors to split up the list of pending
patches and then look at each one with an eye to what needs to
happen to move it along (*not* to commit it right away, although
in some cases maybe that's the thing to do).  It'd be great if
that could happen just before each commitfest, but that's probably
not practical with the current patch volume.  What I'm thinking
for the moment is to try to make that happen once a year or so.

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

Yeah, all this stuff ultimately gets done "for the good of the
project", which isn't the most reliable motivation perhaps.
I don't see a better one...

            regards, tom lane

On Sun, May 19, 2024 at 03:18:11PM -0400, Tom Lane wrote:
> * Another reason for things sitting a long time is that they're too
> big to review without an unreasonable amount of effort.  We should
> encourage authors to break large patches into smaller stepwise
> refinements.  It may seem like that will result in taking forever
> to reach the end goal, but dropping a huge patchset on the community
> isn't going to give speedy results either.

I think it is sometimes hard to incrementally apply patches if the
long-term goal isn't agreed or know to be achievable.

> * Before starting this thread, Robert did a lot of very valuable
> review of some individual patches.  I think what prompted him to
> start the thread was the realization that he'd only made a small
> dent in the problem.  Maybe we could divide and conquer: get a
> dozen-or-so senior contributors to split up the list of pending
> patches and then look at each one with an eye to what needs to
> happen to move it along (*not* to commit it right away, although
> in some cases maybe that's the thing to do).  It'd be great if
> that could happen just before each commitfest, but that's probably
> not practical with the current patch volume.  What I'm thinking
> for the moment is to try to make that happen once a year or so.

For me, if someone already knows what the blocker is, it saves me a lot
of time if they can state that somewhere.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

Bruce Momjian <bruce@momjian.us> writes:
> On Sun, May 19, 2024 at 03:18:11PM -0400, Tom Lane wrote:
>> * Another reason for things sitting a long time is that they're too
>> big to review without an unreasonable amount of effort.  We should
>> encourage authors to break large patches into smaller stepwise
>> refinements.  It may seem like that will result in taking forever
>> to reach the end goal, but dropping a huge patchset on the community
>> isn't going to give speedy results either.

> I think it is sometimes hard to incrementally apply patches if the
> long-term goal isn't agreed or know to be achievable.

True.  The value of the earlier patches in the series can be unclear
if you don't understand what the end goal is.  But I think people
could post a "road map" of how they intend a patch series to go.

Another way of looking at this is that sometimes people do post large
chunks of work in long many-patch sets, but we tend to look at the
whole series as something to review and commit as one (or I do, at
least).  We should be more willing to bite off and push the earlier
patches in such a series even when the later ones aren't entirely
done.

(The cfbot tends to discourage this, since as soon as one of the
patches is committed it no longer knows how to apply the rest.
Can we improve on that tooling somehow?)

            regards, tom lane

On Mon, May 20, 2024 at 1:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> (The cfbot tends to discourage this, since as soon as one of the
> patches is committed it no longer knows how to apply the rest.
> Can we improve on that tooling somehow?)

Cfbot currently applies patches with  (GNU) patch
--no-backup-if-mismatch -p1 -V none -f.  The -f means that any
questions of the form "Reversed (or previously applied) patch
detected!  Assume -R? [y]" is answered with "yes", and the operation
fails.  I wondered if --forward would be better.  It's the right idea,
but it seems to be useless in practice for this purpose because the
command still fails:

tmunro@phonebox postgresql % patch --forward -p1 < x.patch || echo XXX
failed $?
patching file 'src/backend/postmaster/postmaster.c'
Ignoring previously applied (or reversed) patch.
1 out of 1 hunks ignored--saving rejects to
'src/backend/postmaster/postmaster.c.rej'
XXX failed 1

I wondered if it might be distinguishable from other kinds of failure
that should stop progress, but nope:

       patch's exit status is 0 if all hunks are applied successfully, 1
       if some hunks cannot be applied or there were merge conflicts,
       and 2 if there is more serious trouble.  When applying a set of
       patches in a loop it behooves you to check this exit status so
       you don't apply a later patch to a partially patched file.

I guess I could parse stdout or whatever that is and detect
all-hunks-ignored condition, but that doesn't sound like fun...

Perhaps cfbot should test explicitly for patches that have already
been applied with something like "git apply --reverse --check", and
skip them.  That would work for exact patches, but of course it would
be confused by any tweaks made before committing.  If going that way,
it might make sense to switch to git apply/am (instead of GNU patch),
to avoid contradictory conclusions.

The reason I was using GNU patch in the first place is that it is/was
a little more tolerant of some of the patches people used to send a
few years back, but now I expect everyone uses git format-patch and
would be prepared to change their ways if not.  In the past we had a
couple of cases of the reverse, that is, GNU patch couldn't apply
something that format-patch produced (some edge case of renaming,
IIRC) and I'm sorry that I never got around to changing that.

Sometimes I question the sanity of the whole thing.  Considering
cfbot's original "zero-effort CI" goal (or I guess "zero-extra-effort"
would be better), I was curious about what other projects had the same
idea, or whether we're really just starting at the "wrong end", and
came up with:

https://github.com/getpatchwork/patchwork
http://vger.kernel.org/bpfconf2022_material/lsfmmbpf2022-bpf-ci.pdf
<-- example user
https://github.com/patchew-project/patchew

Actually cfbot requires more effort than those, because it's driven
first by Commitfest app registration.  Those projects are extremists
IIUC: just write to a mailing list, no other bureaucracy at all (at
least for most participants, presumably administrators can adjust the
status in some database when things go wrong?).  We're actually
halfway to Gitlab et al already, with a web account and interaction
required to start the process of submitting a patch for consideration.
What I'm less clear on is who else has come up with the "bitrot" test
idea, either at the mailing list or web extremist ends of the scale.
Those are also generic tools, and cfbot obviously knows lots of things
about PostgreSQL, like the "highlights" and probably more things I'm
forgetting.

> On 20 May 2024, at 07:49, Thomas Munro <thomas.munro@gmail.com> wrote:

> We're actually
> halfway to Gitlab et al already, with a web account and interaction
> required to start the process of submitting a patch for consideration.

Another Web<->Mailinglist extreme is Git themselves who have a Github bridge
for integration with their usual patch-on-mailinglist workflow.

https://github.com/gitgitgadget/gitgitgadget

> What I'm less clear on is who else has come up with the "bitrot" test
> idea, either at the mailing list or web extremist ends of the scale.

Most web based platforms like Github register the patch against the tree at the
time of submitting, and won't refresh unless the user does so.  Github does
detect bitrot and show a "this cannot be merged" error message, but it doesn't
alter any state on the PR etc.

--
Daniel Gustafsson

On 20/05/2024 02:09, Tom Lane wrote:

> Bruce Momjian <bruce@momjian.us> writes:
>> On Sun, May 19, 2024 at 03:18:11PM -0400, Tom Lane wrote:
>>> * Another reason for things sitting a long time is that they're too
>>> big to review without an unreasonable amount of effort.  We should
>>> encourage authors to break large patches into smaller stepwise
>>> refinements.  It may seem like that will result in taking forever
>>> to reach the end goal, but dropping a huge patchset on the community
>>> isn't going to give speedy results either.
> 
>> I think it is sometimes hard to incrementally apply patches if the
>> long-term goal isn't agreed or know to be achievable.
> 
> True.  The value of the earlier patches in the series can be unclear
> if you don't understand what the end goal is.  But I think people
> could post a "road map" of how they intend a patch series to go.
> 
> Another way of looking at this is that sometimes people do post large
> chunks of work in long many-patch sets, but we tend to look at the
> whole series as something to review and commit as one (or I do, at
> least).  We should be more willing to bite off and push the earlier
> patches in such a series even when the later ones aren't entirely
> done.

[resend due to DKIM header failure]

Right. As an observation from someone who used to dabble in PostgreSQL internals a 
number of years ago (and who now spends a lot of time working on other well-known 
projects), this is something that really stands out with the current PostgreSQL workflow.

In general you find that a series consists of 2 parts: 1) a set of refactorings to 
enable a new feature and 2) the new feature itself. Even if the details of 2) are 
still under discussion, often it is possible to merge 1) fairly quickly which also 
has the knock-on effect of reducing the size of later iterations of the series. This 
also helps with new contributors since having parts of the series merged sooner helps 
them feel valued and helps to provide immediate feedback.

The other issue I mentioned last time this discussion arose is that I really miss the 
standard email-based git workflow for PostgreSQL: writing a versioned cover letter 
helps reviewers as the summary provides a list of changes since the last iteration, 
and having separate emails with a PATCH prefix allows patches to be located quickly.

Finally as a reviewer I find that having contributors use git format-patch and 
send-email makes it easier for me to contribute, since I can simply hit "Reply" and 
add in-line comments for the parts of the patch I feel I can review. At the moment I 
have to locate the emails that contain patches and save the attachments before I can 
even get to starting the review process, making the initial review barrier that 
little bit higher.


ATB,

Mark.

On 20/05/2024 06:56, Mark Cave-Ayland wrote:

> In general you find that a series consists of 2 parts: 1) a set of refactorings to 
> enable a new feature and 2) the new feature itself. Even if the details of 2) are 
> still under discussion, often it is possible to merge 1) fairly quickly which also 
> has the knock-on effect of reducing the size of later iterations of the series. This 
> also helps with new contributors since having parts of the series merged sooner helps 
> them feel valued and helps to provide immediate feedback.

[resend due to DKIM header failure]

Something else I also notice is that PostgreSQL doesn't have a MAINTAINERS or 
equivalent file, so when submitting patches it's difficult to know who is expected to 
review and/or commit changes to a particular part of the codebase (this is true both 
with and without the CF system).


ATB,

Mark.

On 2024-May-19, Tom Lane wrote:

> (The cfbot tends to discourage this, since as soon as one of the
> patches is committed it no longer knows how to apply the rest.
> Can we improve on that tooling somehow?)

I think a necessary next step to further improve the cfbot is to get it
integrated in pginfra.  Right now it runs somewhere in Thomas's servers
or something, and there's no real integration with the commitfest proper
except by scraping.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)

On Sun, May 19, 2024 at 3:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dmitry Dolgov <9erthalion6@gmail.com> writes:
> > How do we make sure this time it will be different?
>
> Things *have* changed, if you take the long view.

That's true, but I think Dmitry's point is well-taken all the same: we
haven't really made a significant process improvement in many years,
and in some ways, I think things have been slowly degrading. I don't
believe it's necessary or possible to solve all of the accumulated
problems overnight, but we need to get serious about admitting that
there is a problem which, in my opinion, is an existential threat to
the project.

> IMV one really fundamental problem is the same as it's been for a
> couple of decades: too many patch submissions, too few committers.
> We can't fix that by just appointing a ton more committers, at least
> not if we want to keep the project's quality up.  We have to grow
> qualified committers.  IIRC, one of the main reasons for instituting
> the commitfests at all was the hope that if we got more people to
> spend time reading the code and reviewing patches, some of them would
> learn enough to become good committers.  I think that's worked, again
> taking a long view.  I just did some quick statistics on the commit
> history, and I see that we were hovering at somewhere around ten
> active committers from 1999 to 2012, but since then it's slowly crept
> up to about two dozen today.  (I'm counting "active" as "at least 10
> commits per year", which is an arbitrary cutoff --- feel free to slice
> the data for yourself.)  Meanwhile the number of submissions has also
> grown, so I'm not sure how much better the load ratio is.

That's an interesting statistic. I had not realized that the numbers
had actually grown significantly. However, I think that the
new-patch-submitter experience has not improved; if anything, I think
it may have gotten worse. It's hard to compare the subjective
experience between 2008 when I first got involved and now, especially
since at that time I was a rank newcomer experiencing things as a
newcomer, and now I'm a long-time committer trying to judge the
newcomer experience. But it seems to me that when I started, there was
more of a "middle tier" of people who were not committers but could do
meaningful review of patches and help you push them in the direction
of being something that a committer might not loathe. Now, I feel like
there are a lot of non-committer reviews that aren't actually adding a
lot of value: people come along and say the patch doesn't apply, or a
word is spelled wrong, and we don't get meaningful review of whether
the design makes sense, or if we do, it's wrong. Perhaps this is just
viewing the past with rose-colored glasses: I wasn't in as good a
position to judge the value of reviews that I gave and received at
that point as I am now. But what I do think is happening today is that
a lot of committer energy gets focused on the promising non-committers
who someone thinks might be able to become committers, and other
non-committers struggle to make any headway.

On the plus side, I think we make more of an effort not to be a jerk
to newcomers than we used to. I also have a strong hunch that it may
not be as good as it needs to be.

> * Patches that sit in the queue a long time tend to be ones that lack
> consensus, either about the goal or the details of how to achieve it.
> Sometimes "lacks consensus" really means "nobody but the author thinks
> this is a good idea, but we haven't mustered the will to say no".
> But I think it's more usually the case that there are plausible
> competing opinions about what the patch should do or how it should
> do it.  How can we resolve such differences and get something done?

This is a great question. We need to do better with that.

Also, it would be helpful to have better ways of handling it when the
author has gotten to a certain point with it but doesn't necessarily
have the time/skills/whatever to drive it forward. Such patches are
quite often a good idea, but it's not clear what we can do with them
procedurally other than hit the reject button, which doesn't feel
great.

> * Another reason for things sitting a long time is that they're too
> big to review without an unreasonable amount of effort.  We should
> encourage authors to break large patches into smaller stepwise
> refinements.  It may seem like that will result in taking forever
> to reach the end goal, but dropping a huge patchset on the community
> isn't going to give speedy results either.

Especially if it has a high rate of subtle defects, which is common.

> * Before starting this thread, Robert did a lot of very valuable
> review of some individual patches.  I think what prompted him to
> start the thread was the realization that he'd only made a small
> dent in the problem.  Maybe we could divide and conquer: get a
> dozen-or-so senior contributors to split up the list of pending
> patches and then look at each one with an eye to what needs to
> happen to move it along (*not* to commit it right away, although
> in some cases maybe that's the thing to do).  It'd be great if
> that could happen just before each commitfest, but that's probably
> not practical with the current patch volume.  What I'm thinking
> for the moment is to try to make that happen once a year or so.

I like this idea.

> Yeah, all this stuff ultimately gets done "for the good of the
> project", which isn't the most reliable motivation perhaps.
> I don't see a better one...

This is the really hard part.

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

On Mon, May 20, 2024 at 7:49 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2024-May-19, Tom Lane wrote:
> > (The cfbot tends to discourage this, since as soon as one of the
> > patches is committed it no longer knows how to apply the rest.
> > Can we improve on that tooling somehow?)
>
> I think a necessary next step to further improve the cfbot is to get it
> integrated in pginfra.  Right now it runs somewhere in Thomas's servers
> or something, and there's no real integration with the commitfest proper
> except by scraping.

Yes, I think we really need to fix this. Also, there's a bunch of
mechanical work that could be done to make cfbot better, like making
the navigation not reset the scroll every time you drill down one
level through the build products.

I would also like to see the buildfarm and CI converged in some way.
I'm not sure how. I understand that the buildfarm tests more different
configurations than we can reasonably afford to do in CI, but there is
no sense in pretending that having two different systems doing similar
jobs has no cost.

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

On Fri, 17 May 2024 at 15:02, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 17.05.24 09:32, Heikki Linnakangas wrote:
> > 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.
>
> We already have the annotations feature, which is kind of this.

But annotations are bound to mails in attached mail threads, rather
than a generic text input at the CF entry level. There isn't always an
appropriate link between (mail or in-person) conversations about the
patch, and a summary of that conversation.

----

The CommitFest App has several features, but contains little
information about how we're expected to use it. To start addressing
this limitation, I've just created a wiki page about the CFA [0], with
a handbook section. Feel free to extend or update the information as
appropriate; I've only added that information the best of my
knowledge, so it may contain wrong, incomplete and/or inaccurate
information.

Kind regards,

Matthias van de Meent

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

On Sun, May 19, 2024 at 10:50 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> Sometimes I question the sanity of the whole thing.  Considering
> cfbot's original "zero-effort CI" goal (or I guess "zero-extra-effort"
> would be better), I was curious about what other projects had the same
> idea, or whether we're really just starting at the "wrong end", and
> came up with:
>
> https://github.com/getpatchwork/patchwork
> http://vger.kernel.org/bpfconf2022_material/lsfmmbpf2022-bpf-ci.pdf
> <-- example user
> https://github.com/patchew-project/patchew
>
> Actually cfbot requires more effort than those, because it's driven
> first by Commitfest app registration.  Those projects are extremists
> IIUC: just write to a mailing list, no other bureaucracy at all (at
> least for most participants, presumably administrators can adjust the
> status in some database when things go wrong?).  We're actually
> halfway to Gitlab et al already, with a web account and interaction
> required to start the process of submitting a patch for consideration.
> What I'm less clear on is who else has come up with the "bitrot" test
> idea, either at the mailing list or web extremist ends of the scale.
> Those are also generic tools, and cfbot obviously knows lots of things
> about PostgreSQL, like the "highlights" and probably more things I'm
> forgetting.

For what it's worth, a few years before cfbot, I had privately
attempted a similar idea for Postgres [1]. The project here is
basically a very simple API and infrastructure for running builds and
make check. A previous version [2] subscribed to the mailing lists and
used Travis CI (and accidentally spammed some Postgres committers
[3]). The project petered out as my work responsibilities shifted (and
to be honest, after I felt sheepish about the spamming).

I think cfbot is way, way ahead of where my project got at this point.
But since you asked about other similar projects, I'm happy to discuss
further if it's helpful to bounce ideas off someone who's thought
about the same problem (though not for a while now, I admit).

Thanks,
Maciek

[1]: https://github.com/msakrejda/pg-quilter
[2]:
https://github.com/msakrejda/pg-quilter/blob/2038d9493f9aa7d43d3eb0aec1d299b94624602e/lib/pg-quilter/git_harness.rb
[3]:
https://www.postgresql.org/message-id/flat/CAM3SWZQboGoVYAJNoPMx%3DuDLE%2BZh5k2MQa4dWk91YPGDxuY-gQ%40mail.gmail.com#e24bf57b77cfb6c440c999c018c46e92

On 5/20/24 16:16, Robert Haas wrote:
> On Sun, May 19, 2024 at 3:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>...
> 
>> * Another reason for things sitting a long time is that they're too
>> big to review without an unreasonable amount of effort.  We should
>> encourage authors to break large patches into smaller stepwise
>> refinements.  It may seem like that will result in taking forever
>> to reach the end goal, but dropping a huge patchset on the community
>> isn't going to give speedy results either.
> 
> Especially if it has a high rate of subtle defects, which is common.
> 

I think breaking large patches into reasonably small parts is a very
important thing. Not only is it really difficult (or perhaps even
practically impossible) to review patches over a certain size, because
you have to grok and review everything at once. But it's also not great
from the cost/benefit POV - the improvement may be very beneficial, but
if it's one huge lump of code that never gets committable as a whole,
there's no benefit in practice. Which makes it less likely I'll even
start looking at the patch very closely, because there's a risk it'd be
just a waste of time in the end.

So I think this is another important reason to advise people to split
patches into smaller parts - not only it makes it easier to review, it
makes it possible to review and commit the parts incrementally, getting
at least some benefits early.

>> * Before starting this thread, Robert did a lot of very valuable
>> review of some individual patches.  I think what prompted him to
>> start the thread was the realization that he'd only made a small
>> dent in the problem.  Maybe we could divide and conquer: get a
>> dozen-or-so senior contributors to split up the list of pending
>> patches and then look at each one with an eye to what needs to
>> happen to move it along (*not* to commit it right away, although
>> in some cases maybe that's the thing to do).  It'd be great if
>> that could happen just before each commitfest, but that's probably
>> not practical with the current patch volume.  What I'm thinking
>> for the moment is to try to make that happen once a year or so.
> 
> I like this idea.
> 

Me too. Do you think it'd happen throughout the whole year, or at some
particular moment?

We used to do a "triage" at the FOSDEM PGDay meeting, but that used to
be more of an ad hoc thing to use the remaining time, with only a small
subset of people. But that seems pretty late in the dev cycle, I guess
we'd want it to happen early, perhaps during the first CF?

FWIW this reminds me the "CAN reports" tracking the current "conditions,
actions and needs" of a ticket. I do that for internal stuff, and I find
that quite helpful (as long as it's kept up to date).

>> Yeah, all this stuff ultimately gets done "for the good of the
>> project", which isn't the most reliable motivation perhaps.
>> I don't see a better one...
> 
> This is the really hard part.
> 

I think we have plenty of motivated people with good intentions. Some
are motivated by personal interest, some by trying to achieve stuff
related to their work/research/... I don't think the exact reasons
matter too much, and it's often a combination.

IMHO we should look at this from the other end - people are motivated to
get a patch reviewed & committed, and if we introduce a process that's
more likely to lead to that result, people will mostly adopt that.

And if we could make that process more convenient by improving the CF
app to support it, that'd be even better ... I'm mostly used to the
mailing list idea, but with the volume it's a constant struggle to keep
track of new patch versions that I wanted/promised to review, etc. The
CF app helps with that a little bit, because I can "become a reviewer"
but I still don't get notifications or anything like that :-(


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> On 5/20/24 16:16, Robert Haas wrote:
>> On Sun, May 19, 2024 at 3:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> * Before starting this thread, Robert did a lot of very valuable
>>> review of some individual patches.  I think what prompted him to
>>> start the thread was the realization that he'd only made a small
>>> dent in the problem.  Maybe we could divide and conquer: get a
>>> dozen-or-so senior contributors to split up the list of pending
>>> patches and then look at each one with an eye to what needs to
>>> happen to move it along (*not* to commit it right away, although
>>> in some cases maybe that's the thing to do).  It'd be great if
>>> that could happen just before each commitfest, but that's probably
>>> not practical with the current patch volume.  What I'm thinking
>>> for the moment is to try to make that happen once a year or so.

>> I like this idea.

> Me too. Do you think it'd happen throughout the whole year, or at some
> particular moment?

I was imagining a focused community effort spanning a few days to
a week.  It seems more likely to actually happen if we attack it
that way than if it's spread out as something people will do when
they get around to it.  Of course the downside is finding a week
when everybody is available.

> We used to do a "triage" at the FOSDEM PGDay meeting, but that used to
> be more of an ad hoc thing to use the remaining time, with only a small
> subset of people. But that seems pretty late in the dev cycle, I guess
> we'd want it to happen early, perhaps during the first CF?

Yeah, early in the cycle seems more useful, although the summer might
be the worst time for peoples' availability.

            regards, tom lane

On 5/24/24 19:09, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>> On 5/20/24 16:16, Robert Haas wrote:
>>> On Sun, May 19, 2024 at 3:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> * Before starting this thread, Robert did a lot of very valuable
>>>> review of some individual patches.  I think what prompted him to
>>>> start the thread was the realization that he'd only made a small
>>>> dent in the problem.  Maybe we could divide and conquer: get a
>>>> dozen-or-so senior contributors to split up the list of pending
>>>> patches and then look at each one with an eye to what needs to
>>>> happen to move it along (*not* to commit it right away, although
>>>> in some cases maybe that's the thing to do).  It'd be great if
>>>> that could happen just before each commitfest, but that's probably
>>>> not practical with the current patch volume.  What I'm thinking
>>>> for the moment is to try to make that happen once a year or so.
> 
>>> I like this idea.
> 
>> Me too. Do you think it'd happen throughout the whole year, or at some
>> particular moment?
> 
> I was imagining a focused community effort spanning a few days to
> a week.  It seems more likely to actually happen if we attack it
> that way than if it's spread out as something people will do when
> they get around to it.  Of course the downside is finding a week
> when everybody is available.
> 
>> We used to do a "triage" at the FOSDEM PGDay meeting, but that used to
>> be more of an ad hoc thing to use the remaining time, with only a small
>> subset of people. But that seems pretty late in the dev cycle, I guess
>> we'd want it to happen early, perhaps during the first CF?
> 
> Yeah, early in the cycle seems more useful, although the summer might
> be the worst time for peoples' availability.
> 

I think meeting all these conditions - a week early in the cycle, but
not in the summer, when everyone can focus on this - will be difficult.

If we give up on everyone doing it at the same time, summer would be a
good time to do this - it's early in the cycle, and it tends to be a
quieter period (customers are on vacation too, so fewer incidents).

So maybe it'd be better to just set some deadline by which this needs to
be done, and make sure every pending patch has someone expected to look
at it? IMHO we're not in position to assign stuff to people, so I guess
people would just volunteer anyway - the CF app might track this.

It's not entirely clear to me if this would effectively mean doing a
regular review of those patches, or something less time consuming.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> On 5/24/24 19:09, Tom Lane wrote:
>>>> ... Maybe we could divide and conquer: get a
>>>> dozen-or-so senior contributors to split up the list of pending
>>>> patches and then look at each one with an eye to what needs to
>>>> happen to move it along (*not* to commit it right away, although
>>>> in some cases maybe that's the thing to do).

> I think meeting all these conditions - a week early in the cycle, but
> not in the summer, when everyone can focus on this - will be difficult.

True.  Perhaps in the fall there'd be a better chance?

> So maybe it'd be better to just set some deadline by which this needs to
> be done, and make sure every pending patch has someone expected to look
> at it? IMHO we're not in position to assign stuff to people, so I guess
> people would just volunteer anyway - the CF app might track this.

One problem with a time-extended process is that the set of CF entries
is not static, so a predetermined division of labor will result in
missing some newly-arrived entries.  Maybe that's not a problem
though; anything newly-arrived is by definition not "stuck".  But we
would definitely need some support for keeping track of what's been
looked at and what remains, whereas if it happens over just a few
days that's probably not so essential.

> It's not entirely clear to me if this would effectively mean doing a
> regular review of those patches, or something less time consuming.

I was *not* proposing doing a regular review, unless of course
somebody really wants to do that.  What I am thinking about is
suggesting how to make progress on patches that are stuck, or in some
cases delivering the bad news that this patch seems unlikely to ever
get accepted and it's time to cut our losses.  (Patches that seem to
be moving along in good order probably don't need any attention in
this process, beyond determining that that's the case.)  That's why
I think we need some senior people doing this, as their opinions are
more likely to be taken seriously.

            regards, tom lane

On 5/24/24 15:45, Tom Lane wrote:
> I was *not* proposing doing a regular review, unless of course
> somebody really wants to do that.  What I am thinking about is
> suggesting how to make progress on patches that are stuck, or in some
> cases delivering the bad news that this patch seems unlikely to ever
> get accepted and it's time to cut our losses.  (Patches that seem to
> be moving along in good order probably don't need any attention in
> this process, beyond determining that that's the case.)  That's why
> I think we need some senior people doing this, as their opinions are
> more likely to be taken seriously.

Maybe do a FOSDEM-style dev meeting with triage review at PG.EU would at 
least move us forward? Granted it is less early and perhaps less often 
than the thread seems to indicate, but has been tossed around before and 
seems doable.

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

Joe Conway <mail@joeconway.com> writes:
> On 5/24/24 15:45, Tom Lane wrote:
>> I was *not* proposing doing a regular review, unless of course
>> somebody really wants to do that.  What I am thinking about is
>> suggesting how to make progress on patches that are stuck, or in some
>> cases delivering the bad news that this patch seems unlikely to ever
>> get accepted and it's time to cut our losses.  (Patches that seem to
>> be moving along in good order probably don't need any attention in
>> this process, beyond determining that that's the case.)  That's why
>> I think we need some senior people doing this, as their opinions are
>> more likely to be taken seriously.

> Maybe do a FOSDEM-style dev meeting with triage review at PG.EU would at 
> least move us forward? Granted it is less early and perhaps less often 
> than the thread seems to indicate, but has been tossed around before and 
> seems doable.

Perhaps.  The throughput of an N-person meeting is (at least) a factor
of N less than the same N people looking at patches individually.
On the other hand, the consensus of a meeting is more likely to be
taken seriously than a single person's opinion, senior or not.
So it could work, but I think we'd need some prefiltering so that
the meeting only spends time on those patches already identified as
needing help.

            regards, tom lane

On 5/24/24 22:44, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> On 5/24/24 15:45, Tom Lane wrote:
>>> I was *not* proposing doing a regular review, unless of course
>>> somebody really wants to do that.  What I am thinking about is
>>> suggesting how to make progress on patches that are stuck, or in some
>>> cases delivering the bad news that this patch seems unlikely to ever
>>> get accepted and it's time to cut our losses.  (Patches that seem to
>>> be moving along in good order probably don't need any attention in
>>> this process, beyond determining that that's the case.)  That's why
>>> I think we need some senior people doing this, as their opinions are
>>> more likely to be taken seriously.
> 
>> Maybe do a FOSDEM-style dev meeting with triage review at PG.EU would at 
>> least move us forward? Granted it is less early and perhaps less often 
>> than the thread seems to indicate, but has been tossed around before and 
>> seems doable.
> 
> Perhaps.  The throughput of an N-person meeting is (at least) a factor
> of N less than the same N people looking at patches individually.
> On the other hand, the consensus of a meeting is more likely to be
> taken seriously than a single person's opinion, senior or not.
> So it could work, but I think we'd need some prefiltering so that
> the meeting only spends time on those patches already identified as
> needing help.
> 

I personally don't think the FOSDEM triage is a very productive use of
our time - we go through patches top to bottom, often with little idea
what the current state of the patch is. We always ran out of time after
looking at maybe 1/10 of the list.

Having an in-person discussion about patches would be good, but I think
we should split the meeting into much smaller groups for that, each
looking at a different subset. And maybe it should be determined in
advance, so that people can look at those patches in advance ...


-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> I personally don't think the FOSDEM triage is a very productive use of
> our time - we go through patches top to bottom, often with little idea
> what the current state of the patch is. We always ran out of time after
> looking at maybe 1/10 of the list.

> Having an in-person discussion about patches would be good, but I think
> we should split the meeting into much smaller groups for that, each
> looking at a different subset. And maybe it should be determined in
> advance, so that people can look at those patches in advance ...

Yeah, subgroups of 3 or 4 people sounds about right.  And definitely
some advance looking to see which patches need discussion.

            regards, tom lane

On Thu, May 16, 2024 at 4:00 PM Joe Conway <mail@joeconway.com> wrote:
>
> On 5/16/24 17:36, Jacob Champion wrote:
> > On Thu, May 16, 2024 at 2:29 PM Joe Conway <mail@joeconway.com> wrote:
> >> If no one, including the author (new or otherwise) is interested in
> >> shepherding a particular patch, what chance does it have of ever getting
> >> committed?
> >
> > That's a very different thing from what I think will actually happen, which is
> >
> > - new author posts patch
> > - community member says "use commitfest!"
>
> Here is where we should point them at something that explains the care
> and feeding requirements to successfully grow a patch into a commit.
>
> > - new author registers patch
> > - no one reviews it
> > - patch gets automatically booted
>
> Part of the care and feeding instructions should be a warning regarding
> what happens if you are unsuccessful in the first CF and still want to
> see it through.
>
> > - community member says "register it again!"
> > - new author says ಠ_ಠ
>
> As long as this is not a surprise ending, I don't see the issue.

I've experienced this in another large open-source project that runs
on Github, not mailing lists, and here's how it goes:

1. I open a PR with a small bugfix (test case included).
2. PR is completely ignored by committers (presumably because they all
mostly work on their own projects they're getting paid to do).
3. <3 months goes by>
4. I may get a comment with "please rebase!", or, more frequently, a
bot closes the issue.

That cycle is _infuriating_ as a contributor. As much as I don't like
to hear "we're not doing this", I'd far prefer to have that outcome
then some automated process closing out my submission without my input
when, as far as I can tell, the real problem is not my lack of
activity by the required reviewers simply not looking at it.

So I'm genuinely confused by you say "As long as this is not a
surprise ending, I don't see the issue.". Perhaps we're imagining
something different here reading between the lines?

Regards,
James Coleman

On Fri, May 17, 2024 at 9:59 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, May 17, 2024 at 11:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > We already have gone back to that model. We just haven't admitted it
> > > yet. And we're never going to get out of it until we find a way to get
> > > the contents of the CommitFest application down to a more reasonable
> > > size and level of complexity. There's just no way everyone's up for
> > > that level of pain. I'm not sure not up for that level of pain.
> >
> > Yeah, we clearly need to get the patch list to a point of
> > manageability, but I don't agree that abandoning time-boxed CFs
> > will improve anything.
>
> I'm not sure. Suppose we plotted commits generally, or commits of
> non-committer patches, or reviews on-list, vs. time. Would we see any
> uptick in activity during CommitFests? Would it vary by committer? I
> don't know. I bet the difference wouldn't be as much as Tom Lane would
> like to see. Realistically, we can't manage how contributors spend
> their time all that much, and trying to do so is largely tilting at
> windmills.
>
> To me, the value of time-based CommitFests is as a vehicle to ensure
> freshness, or cleanup, or whatever word you want to do. If you just
> make a list of things that need attention and keep incrementally
> updating it, eventually for various reasons that list no longer
> reflects your current list of priorities. At some point, you have to
> throw it out and make a new list, or at least that's what always
> happens to me. We've fallen into a system where the default treatment
> of a patch is to be carried over to the next CommitFest and CfMs are
> expected to exert effort to keep patches from getting that default
> treatment when they shouldn't. But that does not scale. We need a
> system where things drop off the list unless somebody makes an effort
> to keep them on the list, and the easiest way to do that is to
> periodically make a *fresh* list that *doesn't* just clone some
> previous list.
>
> I realize that many people here are (rightly!) concerned with
> burdening patch authors with more steps that they have to follow. But
> the current system is serving new patch authors very poorly. If they
> get attention, it's much more likely to be because somebody saw their
> email and wrote back than it is to be because somebody went through
> the CommitFest and found their entry and was like "oh, I should review
> this". Honestly, if we get to a situation where a patch author is sad
> because they have to click a link every 2 months to say "yeah, I'm
> still here, please review my patch," we've already lost the game. That
> person isn't sad because we asked them to click a link. They're sad
> it's already been N * 2 months and nothing has happened.

Yes, this is exactly right.

This is an off-the-wall idea, but what if the inverse is actually what
we need? Suppose there's been a decent amount of activity previously
on the thread, but no new patch version or CF app activity (e.g.,
status changes moving it forward) or maybe even just the emails died
off: perhaps that should trigger a question to the author to see what
they want the status to be -- i.e., "is this still 'needs review', or
is it really 'waiting on author' or 'not my priority right now'?"

It seems possible to me that that would actually remove a lot of the
patches from the current CF when a author simply hasn't had time to
respond yet (I know this is the case for me because the time I have to
work on patches fluctuates significantly), but it might also serve to
highlight patches that simply haven't had any review at all.

I'd like to add a feature to the CF app that shows me my current
patches by status, and I'd also like to have the option to have the CF
app notify me when someone changes the status (I've noticed before
that often a status gets changed without notification on list, and
then I get surprised months later when it's stuck in "waiting on
author"). Do either/both of those seem reasonable to add?

Regards,
James Coleman