commitfest.postgresql.org is no longer fit for purpose

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 Thu, May 16, 2024 at 11:30 AM 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
- 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

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

- 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

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

- patches that actually do need to be reviewed

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

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.

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

Second, we added
CI, which means that there is positive value to registering the patch
in the CommitFest even if committing it is not in the cards.

The new slot retains this benefit.

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.

The new slot wouldn't be subject to this.

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

David J.

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

Thanks,
Maciek

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

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

When I was CFM for a couple cycles I started with the idea that I was going
to try being a hardass and rejecting or RWF all the patches that had gotten
negative reviews and been bounced forward.

Except when I actually went through them I didn't find many. Mostly like
Robert I found perfectly reasonable patches that had received generally
positive reviews and had really complex situations that really needed more
analysis.

I also found a lot of patches that were just not getting any reviews at all
:( and rejecting those didn't feel great....

On Thu, May 16, 2024, 21:48 Tom Lane <tgl@sss.pgh.pa.us> wrote:

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 10:46 PM Melanie Plageman <melanieplageman@gmail.com>
wrote:

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.

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.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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 less angst in moving something in the bimonthly into WIP rather than dropping it outright. There is discussion to be had regarding WIP/patch tracking should we go this direction but even if it is just movIng clutter from one room to another there seems like a clear benefit

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

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

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

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

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

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

--Jacob

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

I use a personal JIRA to track the stuff that I hope makes it into the
codebase, as well as just starring the corresponding emails in the
short-term. Every patch ever submitted sits in the mailing list archive so
I have no real need to preserve git branches with my submitted work on
them. At lot of my work comes down to lucky timing so I'm mostly content
with just pinging my draft patches on the email thread once in a while
hoping there is interest and time from someone. For stuff that I would be
OK committing as submitted I'll add it to the commitfest and wait for
someone to either agree or point out where I can improve things.

Adding both these kinds to WIP appeals to me, particularly with something
akin to a "Collaboration Wanted" category in addition to "Needs Review" for
when I think it is ready, and "Waiting on Author" for stuff that has
pending feedback to resolve - or the author isn't currently fishing for
reviewer time for whatever reason. Ideally there would be no rejections,
only constructive feedback that convinces the author that, whether for now
or forever, the proposed patch should be withdrawn pending some change in
circumstances that suggests the world is ready for it.

David J.

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 Fri, May 17, 2024 at 12:25 AM Maciek Sakrejda <m.sakrejda@gmail.com>
wrote:

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

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

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 box on the commitfest, it would be handy.

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

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

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

--
Daniel Gustafsson

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

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 “System Administration” 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.

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’s CF? 5 each week or so. One per day.

Best regards, Andrey Borodin.

On Friday, May 17, 2024, Joe Conway <mail@joeconway.com> wrote:

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?

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

David J.

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]https://commitfest.postgresql.org/48/4248/. 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, May 17, 2024 at 7:11 AM Tomas Vondra <tomas.vondra@enterprisedb.com>
wrote:

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.

This is a huge problem. I've been in the situation before where I had some
cycles to do a review, but actually finding one to review is
super-difficult. You simply cannot tell without clicking on the link and
wading through the email thread. Granted, it's easy as an
occasional reviewer to simply disregard potential patches if the email
thread is over a certain size, but it's still a lot of work. Having some
sort of summary/status field would be great, even if not everything was
labelled. It would also be nice if simpler patches were NOT picked up by
experienced hackers, as we want to encourage new/inexperienced people, and
having some "easy to review" patches available will help people gain
confidence and grow.

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?

Cheers,
Greg

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

On Fri, May 17, 2024 at 9:29 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

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.

Concretely I'm thinking of modifying our patch flow state diagram to this:

stateDiagram-v2
state "Work In Process (Not Timeboxed)" as WIP {
[*] --> CollaboratorsNeeded : Functional but\nFeedback Needed
[*] --> NeedsReview : Simple Enough for\nSign-Off and Send
[*] --> ReworkInProgress : via Returned With Feedback
CollaboratorsNeeded --> NeedsReview : Collaboration Done\nReady for
Sign-Off
CollaboratorsNeeded --> WaitingOnAuthor : Feedback Given\nBack with
Authors
ReworkInProgress --> ReworkCompleted : Rework Ready\nfor Inspection
ReworkCompleted --> ReworkInProgress : More Changes Needed
ReworkCompleted --> ReadyForCommitter : Requested Rework Confirmed\nTry
Again to Commit
NeedsReview --> ReadyForCommitter : Reviewer and Author\nDeem
Submission Worthy
NeedsReview --> WaitingOnAuthor : Changes Needed
WaitingOnAuthor --> NeedsReview : Changes Made
WaitingOnAuthor --> CollaboratorsNeeded : Need Help Making Changes
WaitingOnAuthor --> Withdrawn : Not Going to Make Changes
Withdrawn --> [*]
}

state "Bi-Monthly Timeboxing" as BIM {
[*] --> CommitPending : Simple Committer Patch
CommitPending --> Committed : Done!
CommitPending --> ChangesNeeded : Minor Feedback Given
CommitPending --> ReturnedWithFeedback : Really should have been WIP
first
ReadyForCommitter --> ChangesNeeded : Able to be fixed\nwithin the
current cycle
ReadyForCommitter --> Committed : Done!
ReadyForCommitter --> ReturnedWithFeedback : Not doable in the current
cycle\nSuitable for rejections as well
ChangesNeeded --> ReadyForCommitter
Committed --> [*]
ReturnedWithFeedback --> [*]
}

This allows for usage of WIP as a collaboration area with the side benefit
of CI.

Patches that have gotten commit cycle feedback don't get lumped back into
Needs Review

There is a short-term parking spot for committer-reviewed patches that just
cannot be pushed at the moment. That should be low volume enough to cover
both quick-fixes and freeze-driven waits.

Collaboration Needed should include a description of what kind of feedback
or help is sought. Even if that is just "first timer seeking guidance".

The above details 5 new categories:

Collaborators Needed - Specialized Needs Review for truly WIP work

Rework Completed - Specialized Needs Review to ensure that patches that got
into the bi-monthly once get priority for getting committed

Commit Pending - Specialized Needs Review for easily fast-tracked patches;
and a parking lot for frozen out patches

Rework in Progress - Parking lot for patches already through the bi-monthly
and currently being reworked most likely for the next bi-monthly

Changes Needed - Specialized Waiting on Author but the expected time period
or effort to perform the changes is low; or the patch is just a high
priority one that deserves to remain in the bi-monthly in order to keep
attention on it. When the author is done the committer is waiting for the
revisions and so it goes right back into ReadyForCommitter.

I removed Rejected here but it could be kept. It seems reasonably rolled
into Returned with Feedback and we shouldn't be rejecting without feedback
anyway. Not enough rejection volume to warrant its own category.

David J.

https://mermaid.live/edit#pako:eNp9VU1z2jAQ_Stbn9pOufToQ2cS0qQcSJiQGQ51D7K12Cr2iuoDwmTy37uyYkzADicj73t-u3pPekkKLTFJE-uEwxslSiOaye57Ru0CZMlKmw3MCBZGF2gtfL7XDp5Ug7l-RvklS0BYWM0W8JIR8O_31z8wmfyAqa5rkWsjnDb2HlGihBRuPRVOaRI15N5lGd3ym1wUG4gl7znCmn3EncI9Y5eq2dYIP0n7soK1NgxfqpImD-s1CJKwRDojeMQ9y58Riy9NUJ_CTgledt4QC1opV0EnIUKHdF9q6au4GbjRhCzmEYU8BGHQyfqYciWUU1Q-0JV3FaPSoxS4UzskprwOf_ZBZayxkfGir77ZqQ5Tcu204wq0upgsKJuR3WK7BadMPWhkanNtEKaVoBLtu60axvP3brXh1UY5h6aV8s-jDRVvoqaa1so0zJPRkznAVSkUgdMQUZH9dOjjzOE1PwYLxCkx5Q1iA0ufN8rasEVsY1cdhlkv92Go0_OqAVO8oeZC4jhmOBjhAX5hvWX0hiEd2ThP8K40Yk8BzZm80wHGA2QCPIMfSwOQw5HRa0Z9xq_VZK7JVfWhSzZTxWRfz-aXyQ7DX3DYwgePuez3ZCFcUXXWP63t0dGfITefRgtjB8cJzRVx52cJGcF2AQ-NHxHBKqLmHm2lfS2hEjuEHJHa44vNaF3n6XOXDem5yrlpHneOjH1ufRyCGkxcIRTeGCQHxaGo8UPa4XkMF4_0FbZfahEEDX6ez0mvXPs-nAEG_8YjwIYd3mNdv83xXYeDeTsdeBf31k9R84C6U8cl35IGTSOU5BundVWWsNoGsyTlRynMJku4kuuEd3p5oCJJnfH4LfFb2V9QSboWteVVlIpTNI9XWHuTvf4HuH5UYg

[image: image.png]

[image: image.png]

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]https://wiki.postgresql.org/wiki/CommitFest_App, 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

Hi everyone!

I would like to share another perspective on this as a relatively new user
of the commitfest app. I really like the concept behind the commitfest app
but while using it, sometimes I feel that we can do a better job at having
some sort of a 'metainfo' for the patch.
Although in some cases the patch title is enough to understand what it is
doing but for new contributors and reviewers it would be really helpful if
we can have something more explanatory instead of just having topics like
'bug fix', 'features' etc. Some sort of a small summarised description for
a patch explaining its history and need in brief would be really helpful
for people to get started instead of trying to make sense of a very large
email thread. This is a small addition but it would definitely make it
easier for new reviewers and contributors.

Regards,
Akshat Jaimini

On Mon, 20 May, 2024, 21:26 Matthias van de Meent, <
boekewurm+postgres@gmail.com> wrote:

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]https://github.com/msakrejda/pg-quilter. The project here is
basically a very simple API and infrastructure for running builds and
make check. A previous version [2]https://github.com/msakrejda/pg-quilter/blob/2038d9493f9aa7d43d3eb0aec1d299b94624602e/lib/pg-quilter/git_harness.rb subscribed to the mailing lists and
used Travis CI (and accidentally spammed some Postgres committers
[3]: /messages/by-id/CAM3SWZQboGoVYAJNoPMx=uDLE+Zh5k2MQa4dWk91YPGDxuY-gQ@mail.gmail.com
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]: /messages/by-id/CAM3SWZQboGoVYAJNoPMx=uDLE+Zh5k2MQa4dWk91YPGDxuY-gQ@mail.gmail.com

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