[Commitfest 2022-07] Patch Triage: Waiting on Author

On Tue, Jul 26, 2022 at 12:26:59PM -0700, Jacob Champion wrote:

Hello all,

I'm making my way through some stalled patches in Waiting on Author. If
nothing changes by the end of this CF, I'd recommend marking these
as Returned with Feedback.

+1

I suggest that, if you send an email when marking as RWF, to mention that the
existing patch record can be re-opened and moved to next CF.

I'm aware that people may think that this isn't always a good idea, but it's
nice to mention that it's possible. It's somewhat comparable to starting a new
thread (preferably including a link to the earlier one).

--
Justin

On 7/26/22 16:20, Justin Pryzby wrote:

I suggest that, if you send an email when marking as RWF, to mention that the
existing patch record can be re-opened and moved to next CF.

I'm aware that people may think that this isn't always a good idea, but it's
nice to mention that it's possible. It's somewhat comparable to starting a new
thread (preferably including a link to the earlier one).

Thanks, will do!

--Jacob

On Tue, Jul 26, 2022 at 3:27 PM Jacob Champion <jchampion@timescale.com> wrote:
...

- Consider parallel for LATERAL subqueries having LIMIT/OFFSET
https://commitfest.postgresql.org/38/2851/

Does this have a path forward? It's been Waiting on Author since the
beginning of last CF, with open concerns from Tom about safety.

...

- Parallelize correlated subqueries that execute within each worker
https://commitfest.postgresql.org/38/3246/

Patch needs to be fixed for FreeBSD; last Author post in January.

These are both mine, and I'd hoped to work on them this CF, but I've
been sufficiently busy that that hasn't happened.

I'd like to just move these to the next CF.

Thanks,
James Coleman

On Tue, Jul 26, 2022 at 7:47 PM James Coleman <jtc331@gmail.com> wrote:

These are both mine, and I'd hoped to work on them this CF, but I've
been sufficiently busy that that hasn't happened.

I'd like to just move these to the next CF.

Well, if we mark them returned with feedback now, and you get time to
work on them, you can always change the status back to something else
at that point.

That has the advantage that, if you don't get time to work on them,
they're not cluttering up the next CF in the meantime.

We're not doing a great job kicking things out of the CF when they are
non-actionable, and thereby we are making life harder for ourselves
collectively.

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

On Wednesday, July 27, 2022 3:27 AM Jacob Champion <jchampion@timescale.com> wrote:

- Parallel INSERT SELECT take 2
https://commitfest.postgresql.org/38/3143/

There was a lot of discussion early on in this patchset's life, and
then it got caught in a rebase loop without review in August 2021. The
thread has mostly gone dark since then and the patch does not apply.

Sorry, I think we don't enough time to work on this recently. So please mark it as RWF and
we will get back to this in the future.

Best regards,
Hou zj

27 июля 2022 г., в 00:26, Jacob Champion <jchampion@timescale.com> написал(а):

- libpq compression
https://commitfest.postgresql.org/38/3499/

Needs a rebase and response to feedback; mostly quiet since January.

Daniil is working on this, but currently he's on vacation.
I think we should not mark patch as RwF and move it to next CF instead.

Thank you!

Best regards, Andrey Borodin.

On Wed, Jul 27, 2022 at 7:09 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

Sorry, I think we don't enough time to work on this recently. So please mark it as RWF and
we will get back to this in the future.

Done, thanks!

--Jacob

On Thu, Jul 28, 2022 at 4:46 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Daniil is working on this, but currently he's on vacation.
I think we should not mark patch as RwF and move it to next CF instead.

Is there a downside to marking it RwF, from your perspective? As
Robert pointed out upthread, it can be switched back at any time once
Daniil's ready.

Life happens; there isn't (or there shouldn't be) any shame in having
a patch returned temporarily. But it is important that patches which
aren't ready for review at the moment don't stick around for months.
They take up reviewer time and need to be triaged continually.

--Jacob

On Thu, Jul 28, 2022 at 08:58:31AM -0700, Jacob Champion wrote:

On Thu, Jul 28, 2022 at 4:46 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Daniil is working on this, but currently he's on vacation.
I think we should not mark patch as RwF and move it to next CF instead.

Is there a downside to marking it RwF, from your perspective? As
Robert pointed out upthread, it can be switched back at any time once
Daniil's ready.

As someone interested in seeing the patch progress, I still think it may be
better to close the patch record, which can be re-opened when it's ready to be
reviewed.

They take up reviewer time and need to be triaged continually.

Alternately:

@Jacob: Is there any reason why it's necessary to do anything at all ?
Does something bad happen if the patches are left in the current CF ?
Why make not let patch authors (re) submit the patch for review when they're
ready? Someone went to the effort to move it to the current CF, even though the
patch wasn't ready to be reviewed. It'd be less work and avoid the process of
"moving patches to the next CF" even though (at least in this case) it maybe
shouldn't have even been in the current CF.

Also, is there a place which lists all of an author's patches (current and
historic)? I think people would be less adverse to having their patches closed
if 1) they knew they could re-open them; and, 2) there were a list of patches
and their disposition (not a separate list per commitfest, and not showing each
patch duplicated for each CF that a patch was opened in).

--
Justin

On 8/1/22 08:51, Justin Pryzby wrote:

@Jacob: Is there any reason why it's necessary to do anything at all ?
Does something bad happen if the patches are left in the current CF ?
Why make not let patch authors (re) submit the patch for review when they're
ready? Someone went to the effort to move it to the current CF, even though the
patch wasn't ready to be reviewed. It'd be less work and avoid the process of
"moving patches to the next CF" even though (at least in this case) it maybe
shouldn't have even been in the current CF.

Maybe this is something to look into once we've implemented some more of
the low-hanging usability features that people have asked for. But if we
started doing it now, I'd expect the CFM's job to simply change from
moving patches ahead to pinging people who have patches left behind,
asking them if they meant to move the patches forward. I'm not convinced
it'd be all that useful.

Also, is there a place which lists all of an author's patches (current and
historic)? I think people would be less adverse to having their patches closed
if 1) they knew they could re-open them; and, 2) there were a list of patches
and their disposition (not a separate list per commitfest, and not showing each
patch duplicated for each CF that a patch was opened in).

This would be great to have. I have a patch in progress that introduces
a "deferred" group, to make it more obvious the difference between a
patch that has been Rejected and a patch that's simply Returned or
Moved. Your suggestion would dovetail nicely with that, to able to see
"all my deferred patches".

--Jacob

On Mon, Aug 1, 2022 at 12:30 PM Jacob Champion <jchampion@timescale.com> wrote:

Maybe this is something to look into once we've implemented some more of
the low-hanging usability features that people have asked for. But if we
started doing it now, I'd expect the CFM's job to simply change from
moving patches ahead to pinging people who have patches left behind,
asking them if they meant to move the patches forward. I'm not convinced
it'd be all that useful.

We really need to move to a system where it's the patch author's job
to take some action if the patch is alive, rather than having the CM
(or any other human being) pinging to find out whether it's dead.
Having the default action for a patch be to carry it along to the next
CF whether or not there are any signs of life is unproductive.

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

On 8/1/22 09:33, Robert Haas wrote:

We really need to move to a system where it's the patch author's job
to take some action if the patch is alive, rather than having the CM
(or any other human being) pinging to find out whether it's dead.> Having the default action for a patch be to carry it along to the next
CF whether or not there are any signs of life is unproductive.

In the medium to long term, I agree with you.

In the short term I want to see the features that help authors keep
their patches alive (cfbot integration! automatic rebase reminders!
automated rebase?) so that we're not just artificially raising the
barrier to entry. People with plenty of time on their hands will be able
to go through the motions of moving their patches ahead regardless of
whether or not the patch is dead.

--Jacob

Jacob Champion <jchampion@timescale.com> writes:

On 8/1/22 09:33, Robert Haas wrote:

We really need to move to a system where it's the patch author's job
to take some action if the patch is alive, rather than having the CM
(or any other human being) pinging to find out whether it's dead.> Having the default action for a patch be to carry it along to the next
CF whether or not there are any signs of life is unproductive.

In the medium to long term, I agree with you.

In the short term I want to see the features that help authors keep
their patches alive (cfbot integration! automatic rebase reminders!
automated rebase?) so that we're not just artificially raising the
barrier to entry. People with plenty of time on their hands will be able
to go through the motions of moving their patches ahead regardless of
whether or not the patch is dead.

Yeah, I don't want to introduce make-work into the process; there's
more than enough real work involved. At minimum, a patch that's
shown signs of life since the previous CF should be auto-advanced
to the next one.

regards, tom lane

On Mon, Aug 1, 2022 at 12:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I don't want to introduce make-work into the process; there's
more than enough real work involved. At minimum, a patch that's
shown signs of life since the previous CF should be auto-advanced
to the next one.

Maybe so, but we routinely have situations where a patch hasn't been
updated in 3-6 months and we tentatively ask the author if it would be
OK to mark it RwF, and they often say something like "please keep it
alive for one more CF to see if I have time to work on it." IMHO, that
creates the pretty ridiculous situation where CFMs are putting time
into patches that the author isn't working on and hasn't worked on in
a long time. The CF list isn't supposed to be a catalog of every patch
somebody's thought about working on at any point in the last few
years; it's supposed to be a list of things that need to be reviewed
for possible commit. That's why it's called a COMMIT-fest.

Back in the day, I booted patches out of the CF if they weren't
updated within 4 days of a review being posted. I guess people found
that too harsh, but now it feels like we've gone awfully far towards
the other extreme.

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

Robert Haas <robertmhaas@gmail.com> writes:

Maybe so, but we routinely have situations where a patch hasn't been
updated in 3-6 months and we tentatively ask the author if it would be
OK to mark it RwF, and they often say something like "please keep it
alive for one more CF to see if I have time to work on it."

Agreed, we don't want that. IMO the CF list isn't primarily a to-do
list for patch authors; it's primarily a to-do list for reviewers and
committers. The case that I'm concerned about here is where an author
submits a patch and, through no fault of his/hers, it goes unreviewed
for multiple CFs. As long as the author is keeping the patch refreshed
per CI testing, I don't think additional work to express interest should
be required from the author.

Now admittedly, at some point we should decide that lack of review
indicates that nobody else cares about this patch, in which case it
should get booted with a "sorry, we're just not interested" resolution.
But that can't happen quickly, because we're just drastically short
of review manpower at all times.

On the other hand, I'm quite willing to convert WOA state into RWF
state quickly. The author can always resubmit, or resurrect the
old CF entry, once they have something new for people to look at.

regards, tom lane

On Mon, Aug 1, 2022 at 1:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

On the other hand, I'm quite willing to convert WOA state into RWF
state quickly. The author can always resubmit, or resurrect the
old CF entry, once they have something new for people to look at.

Right. This is what I'm on about.

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