Thread: Commitfest Update

Since the previous update 30 additional patches have been committed
from the CF. This leaves us with 120 Needs Review and 20 Ready for
Committer. There's only a few days left until the end of the month.

* Add comment about startup process getting a free procState array slot always
* Consistent use of SSL/TLS in docs
* Allow COPY "text" to output a header and add header matching mode to COPY FROM
* Enable SSL library detection via PQsslAttribute
* Fully WAL logged CREATE DATABASE - No Checkpoints
* Skipping logical replication transactions on subscriber side
* Add system view pg_ident_file_mappings
* document the need to analyze partitioned tables
* Expose get_query_def()
* JSON path numeric literal syntax
* use has_privs_for_role for predefined roles
* Support for MERGE
* Column filtering in logical replication
* Corruption during WAL replay
* Doc patch for retryable xacts
* pg_statio_all_tables: several rows per table due to invalid TOAST index
* Parameter for planner estimates of recursive queries
* logical decoding and replication of sequences
* Add relation and block-level filtering to pg_waldump
* pgbench - allow retries on some errors
* pgcrypto: Remove internal padding implementation
* Preserving db/ts/relfilenode OIDs across pg_upgrade
* Add new reloption to views for enabling row level security
* Fix handling of outer GroupingFunc within subqueries
* Fix concurrent deadlock scenario with DROP INDEX on partitioned index
* Fix bogus dependency management for GENERATED expressions
* Fix firing of RI triggers during cross-partition updates of partitioned tables
* Add fix to table_to_xmlschema regex when timestamp has time zone
* ltree_gist indexes broken after pg_upgrade from 12
* ExecTypeSetColNames is fundamentally broken


I'm going to start moving any patches that are Waiting on Author to
the next CF if they made any progress recently.

Patches that are Waiting on Author and haven't had activity in months
-- traditionally they were set to Returned with Feedback. It seems the
feeling these days is to not lose state on them and just move them to
the next CF. I'm not sure that's wise, it ends up just filling up the
list with patches nobody's working on.

-- 
greg

On Wed, Mar 30, 2022 at 2:42 PM Greg Stark <stark@mit.edu> wrote:
> Patches that are Waiting on Author and haven't had activity in months
> -- traditionally they were set to Returned with Feedback. It seems the
> feeling these days is to not lose state on them and just move them to
> the next CF. I'm not sure that's wise, it ends up just filling up the
> list with patches nobody's working on.

Yes, we should mark those Returned with Feedback or some other status
that causes them not to get carried forward. The CF is full of stuff
that isn't likely to get committed any time in the foreseeable future,
and that's really unhelpful.

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

On 30.03.22 20:41, Greg Stark wrote:
> Patches that are Waiting on Author and haven't had activity in months
> -- traditionally they were set to Returned with Feedback. It seems the
> feeling these days is to not lose state on them and just move them to
> the next CF. I'm not sure that's wise, it ends up just filling up the
> list with patches nobody's working on.

If you set them to Returned with Feedback now, they can still be 
reawoken later by setting them to Needs Review and pulling them into the 
then-next commit fest.  That preserves all the state and context.

On Wed, Mar 30, 2022 at 02:41:26PM -0400, Greg Stark wrote:
>
> Patches that are Waiting on Author and haven't had activity in months
> -- traditionally they were set to Returned with Feedback. It seems the
> feeling these days is to not lose state on them and just move them to
> the next CF. I'm not sure that's wise, it ends up just filling up the
> list with patches nobody's working on.

+1 for closing such patches as Returned with Feedback, for the same reasons
Robert and Peter already stated.

Note that I already closed such CF entries during the last commitfest, so
hopefully there shouldn't be too much.  Last time I used "both Waiting on
Author and no activity from the author, since at least the 15th of the month"
as the threshold to close such patches (although I closed them at the beginning
of the next month), as it seems to be the usual (and informal) rule.

On 2022-Mar-31, Julien Rouhaud wrote:

> On Wed, Mar 30, 2022 at 02:41:26PM -0400, Greg Stark wrote:
> >
> > Patches that are Waiting on Author and haven't had activity in months
> > -- traditionally they were set to Returned with Feedback. It seems the
> > feeling these days is to not lose state on them and just move them to
> > the next CF. I'm not sure that's wise, it ends up just filling up the
> > list with patches nobody's working on.
> 
> +1 for closing such patches as Returned with Feedback, for the same reasons
> Robert and Peter already stated.

Feature request for the commitfest app: any time a patch is closed as
RwF, send an automated email replying to the last posted version of the
patch, indicating the state change and the URL of the CF entry.  That
way, if somebody wants to resurrect it later, it's easy to find the CF
entry that needs to be edited.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)

On Thu, 31 Mar 2022 at 12:56, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Mar-31, Julien Rouhaud wrote:
>
> > On Wed, Mar 30, 2022 at 02:41:26PM -0400, Greg Stark wrote:
> > >
> > > Patches that are Waiting on Author and haven't had activity in months
> > > -- traditionally they were set to Returned with Feedback. It seems the
> > > feeling these days is to not lose state on them and just move them to
> > > the next CF. I'm not sure that's wise, it ends up just filling up the
> > > list with patches nobody's working on.
> >
> > +1 for closing such patches as Returned with Feedback, for the same reasons
> > Robert and Peter already stated.
>
> Feature request for the commitfest app: any time a patch is closed as
> RwF, send an automated email replying to the last posted version of the
> patch, indicating the state change and the URL of the CF entry.  That
> way, if somebody wants to resurrect it later, it's easy to find the CF
> entry that needs to be edited.

Can normal users actually punt their own patch to a next commitfest?
All I see for my old patches is the 'change status'-button, and the
available options are not obviously linked to 'change registration to
this or upcoming CF'.
Updating the status of RwF/Rejected patches from previous commitfests
to Open (to the best of my knowledge) doesn't automatically register
it to newer CFs, so 'resurrecting' in the CF application can't really
be done by normal users.

I know that this has happened earlier; where someone re-opened their
old RwF-patches in closed commitfests; after which those patches got
lost in the traffic because they are not open in the current (or
upcoming) commitfests.

- Matthias

On 2022-Mar-31, Matthias van de Meent wrote:

> I know that this has happened earlier; where someone re-opened their
> old RwF-patches in closed commitfests; after which those patches got
> lost in the traffic because they are not open in the current (or
> upcoming) commitfests.

Hmm, it's quite possible that an explicit action "move to next CF" is
required.  I don't really know what actions are allowed, though.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Pavel Borisov <pashkin.elfe@gmail.com> writes:
> In my experience, re-applying an updated patch to a new CF is very easy.
> You can re-attach the existing discussion thread. The only information that
> can be lost is CF-specific fields like reviewer/author which is worth
> re-adding manually.

Yeah.  In fact, it might be a good idea to intentionally *not* bring
forward the old reviewers list, as they may have lost interest.

This reminds me of a point I've been meaning to bring up: it seems to
often happen that someone adds their name as reviewer, but then loses
interest and doesn't do anything more with the patch.  I think that's
problematic because people see that the patch already has a reviewer
and look for something else to do.  Would it be feasible or reasonable
to drop reviewers if they've not commented in the thread in X amount
of time?

            regards, tom lane

On Thu, Mar 31, 2022 at 10:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This reminds me of a point I've been meaning to bring up: it seems to
> often happen that someone adds their name as reviewer, but then loses
> interest and doesn't do anything more with the patch.  I think that's
> problematic because people see that the patch already has a reviewer
> and look for something else to do.  Would it be feasible or reasonable
> to drop reviewers if they've not commented in the thread in X amount
> of time?

In theory, this might cause someone who made a valuable contribution
to the discussion to not get credited in the commit message. But it
probably wouldn't in practice, because I at least always construct the
list of reviewers from the thread, not the CF app, since that tends to
be wildly inaccurate in both directions. So maybe it's fine? Not sure.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Mar 31, 2022 at 10:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... Would it be feasible or reasonable
>> to drop reviewers if they've not commented in the thread in X amount
>> of time?

> In theory, this might cause someone who made a valuable contribution
> to the discussion to not get credited in the commit message. But it
> probably wouldn't in practice, because I at least always construct the
> list of reviewers from the thread, not the CF app, since that tends to
> be wildly inaccurate in both directions. So maybe it's fine? Not sure.

Hmm, I tend to believe what's in the CF app, so maybe I'm dropping the
ball on review credits :-(.  But there are various ways we could implement
this.  One way would be a nagbot that sends private email along the lines
of "you haven't commented on patch X in Y months.  Please remove your name
from the list of reviewers if you don't intend to review it any more."

            regards, tom lane

On 3/31/22 07:37, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Mar 31, 2022 at 10:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> ... Would it be feasible or reasonable
>>> to drop reviewers if they've not commented in the thread in X amount
>>> of time?
> 
>> In theory, this might cause someone who made a valuable contribution
>> to the discussion to not get credited in the commit message. But it
>> probably wouldn't in practice, because I at least always construct the
>> list of reviewers from the thread, not the CF app, since that tends to
>> be wildly inaccurate in both directions. So maybe it's fine? Not sure.
> 
> Hmm, I tend to believe what's in the CF app, so maybe I'm dropping the
> ball on review credits :-(.  But there are various ways we could implement
> this.  One way would be a nagbot that sends private email along the lines
> of "you haven't commented on patch X in Y months.  Please remove your name
> from the list of reviewers if you don't intend to review it any more."

It seems there wasn't a definitive decision here. Are there any
objections to more aggressive pruning of the Reviewers entries? So
committers would need to go through the thread for full attribution,
moving forward.

If there are no objections, I'll start doing that during next Friday's
patch sweep.

--Jacob

On Fri, 8 Jul 2022, 23:41 Jacob Champion, <jchampion@timescale.com> wrote:
>
> On 3/31/22 07:37, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Thu, Mar 31, 2022 at 10:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> ... Would it be feasible or reasonable
>>>> to drop reviewers if they've not commented in the thread in X amount
>>>> of time?
>>
>>> In theory, this might cause someone who made a valuable contribution
>>> to the discussion to not get credited in the commit message. But it
>>> probably wouldn't in practice, because I at least always construct the
>>> list of reviewers from the thread, not the CF app, since that tends to
>>> be wildly inaccurate in both directions. So maybe it's fine? Not sure.
>>
>> Hmm, I tend to believe what's in the CF app, so maybe I'm dropping the
>> ball on review credits :-(.  But there are various ways we could implement
>> this.  One way would be a nagbot that sends private email along the lines
>> of "you haven't commented on patch X in Y months.  Please remove your name
>> from the list of reviewers if you don't intend to review it any more."
>
> It seems there wasn't a definitive decision here. Are there any
> objections to more aggressive pruning of the Reviewers entries? So
> committers would need to go through the thread for full attribution,
> moving forward.
>
> If there are no objections, I'll start doing that during next Friday's
> patch sweep.


No objections, but this adds another item to the implicit commitfest
app user manual, that to the best of my knowledge seems to be mostly
implicit institutional knowledge plus bits of information spread
around the mailing lists.

Do we have an actual manual or otherwise a (single?) place with
documentation on how certain fields of the CFA should be used or
interpreted, so that (new) hackers know what to expect or where to
look?

Examples of information about using the CFA that I couldn't find:
- The Version field may contain a single specific postgresql version
number, 'stable', or nothing. If my patch needs backpatching to all
backbranches, which do I select? The oldest supported PG version, or
'stable'? Related to that: which version is indicated by 'stable'?

- When creating or updating a CF entry, who are responsible for
filling in which fields? May the author assign reviewers/committers,
or should they do so themselves?

- Should the 'git link' be filled with a link to the committed patch
once committed, or is it a general purpose link to share a git
repository with the proposed changes?

- What should (or shoudn't) Annotations be used for?

- What should I expect of the comment / review system of the CFA?
Should I use feature over direct on-list mails?

I have checked the wiki page on becoming a developer [0], but that
page seems woefully outdated with statements like "Commitfests are
scheduled to start on the 15th of the month" which hasn't been true
since 2015. The pages on Commitfests [1] and the Developer FAQ [2]
don't add much help either on how to use the CommitFest app. Even
(parts of) the checklist for the CFM on the wiki [3] still assumes the
old CF app that was last used in 2014: "It's based on the current
CommitFest app (written by Robert Haas), and will change once the new
CF App is done."

I'm not asking anyone to drop all and get all the features of the CFA
documented, but for my almost 2 years of following the -hackers list I
feel like I still haven't gotten a good understanding of the
application that is meant to coordinate the shared state in patch
development, and I think that's a quite a shame.

-Matthias

[0] https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F
[1] https://wiki.postgresql.org/wiki/CommitFest
[2] https://wiki.postgresql.org/wiki/Developer_FAQ
[3] https://wiki.postgresql.org/wiki/CommitFest_Checklist

> On 11 Jul 2022, at 15:07, Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:

> No objections, but this adds another item to the implicit commitfest
> app user manual, that to the best of my knowledge seems to be mostly
> implicit institutional knowledge plus bits of information spread
> around the mailing lists.

That's mostly true yes, which means that anything I write below is to be taken
with n grains of salt as it's my interpretation of said institutional
knowledge.

> Do we have an actual manual or otherwise a (single?) place with
> documentation on how certain fields of the CFA should be used or
> interpreted, so that (new) hackers know what to expect or where to
> look?

We don't AFAIK, but we should.  Either an actual written manual (which may end
up in many tldr folders) or inline help within the app (the latter being my
preference I think).

> Examples of information about using the CFA that I couldn't find:
> - The Version field may contain a single specific postgresql version
> number, 'stable', or nothing. If my patch needs backpatching to all
> backbranches, which do I select? The oldest supported PG version, or
> 'stable'? Related to that: which version is indicated by 'stable'?

I'll refer to the commitmessage from the CF app repo on this:

commit a3bac5922db76efd5b6bb331a7141e9ca3209c4a
Author: Magnus Hagander <magnus@hagander.net>
Date:   Wed Feb 6 21:05:06 2019 +0100

    Add a field to each patch for target version

    This is particularly interesting towards the end of a cycle where it can
    be used to flag patches that are not intended for the current version
    but still needs review.

The thread on -hackers which concluded on adding the field has a lot more of
the reasoning but some quick digging didn't find it.

> - When creating or updating a CF entry, who are responsible for
> filling in which fields? May the author assign reviewers/committers,
> or should they do so themselves?

Reviewers and committers sign themselves up.

> - Should the 'git link' be filled with a link to the committed patch
> once committed, or is it a general purpose link to share a git
> repository with the proposed changes?

The gitlink field is (was?) primarily meant to hold links to external repos for
large patchsets where providing a repo on top of the patches in the thread is
valuable.  An example would be Andres et.al's IO work where being able to
follow the work as it unfolds in a repo is valuable for reviewers.

> - What should (or shoudn't) Annotations be used for?

Annotations are used for indicating that certain emails are specifically
important and/or highlight them as taking specific design decisions etc.  It
can be used for anything that is providing value to the a new reader of the
thread really.

> - What should I expect of the comment / review system of the CFA?
> Should I use feature over direct on-list mails?

I think that's up to personal preference, for reviewers who aren't subscribed
to -hackers it's clearly useful to attach it to the thread.  For anyone already
subscribed and used to corresponding on the mailinglist I would think that's
the easiest option.

> I'm not asking anyone to drop all and get all the features of the CFA
> documented, but for my almost 2 years of following the -hackers list I
> feel like I still haven't gotten a good understanding of the
> application that is meant to coordinate the shared state in patch
> development, and I think that's a quite a shame.

There has been a lot of discussions around how to improve the CF app but they
have to a large extent boiled down to ENOTENOUGHTIME.  I still have my hopes
that we can address these before too long, and adding user documentation is
clearly an important one.

--
Daniel Gustafsson        https://vmware.com/

On Fri, Jul 08, 2022 at 02:41:52PM -0700, Jacob Champion wrote:
> On 3/31/22 07:37, Tom Lane wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> On Thu, Mar 31, 2022 at 10:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> ... Would it be feasible or reasonable
> >>> to drop reviewers if they've not commented in the thread in X amount
> >>> of time?
> > 
> >> In theory, this might cause someone who made a valuable contribution
> >> to the discussion to not get credited in the commit message. But it
> >> probably wouldn't in practice, because I at least always construct the
> >> list of reviewers from the thread, not the CF app, since that tends to
> >> be wildly inaccurate in both directions. So maybe it's fine? Not sure.
> > 
> > Hmm, I tend to believe what's in the CF app, so maybe I'm dropping the
> > ball on review credits :-(.  But there are various ways we could implement
> > this.  One way would be a nagbot that sends private email along the lines
> > of "you haven't commented on patch X in Y months.  Please remove your name
> > from the list of reviewers if you don't intend to review it any more."
> 
> It seems there wasn't a definitive decision here. Are there any
> objections to more aggressive pruning of the Reviewers entries? So
> committers would need to go through the thread for full attribution,
> moving forward.
> 
> If there are no objections, I'll start doing that during next Friday's
> patch sweep.

I think it's fine to update the cfapp fields to reflect reality...

..but a couple updates that I just saw seem wrong.  The reviewers field was
nullified, even though the patches haven't been updated in a long time.
There's nothing new to review.  All this has done is lost information that
someone else (me, in this case) went to the bother of adding.

Also, cfapp has a page for "patches where you are the author", but the cfbot
doesn't, and I think people probably look at cfbot more than the cfapp itself.
So being marked as a reviewer is not very visible even to oneself.
But, one of the cfbot patches I sent to Thomas would change that.  Each user's
page would *also* show patches where they're a reviewer ("Needs review -
Reviewer").  That maybe provides an incentive to 1) help maintain the patch; or
otherwise 2) remove oneself.

Also, TBH, this seems to create a lot of busywork.  I'd prefer to see someone
pick one of the patches that hasn't seen a review in 6 (or 16) months, and send
out their most critical review and recommend it be closed, or send an updated
patch with their own fixes as an 0099 patch.

-- 
Justin

On 7/15/22 14:57, Justin Pryzby wrote:
> On Fri, Jul 08, 2022 at 02:41:52PM -0700, Jacob Champion wrote:
>
>> If there are no objections, I'll start doing that during next Friday's
>> patch sweep.
> 
> I think it's fine to update the cfapp fields to reflect reality...
> 
> ..but a couple updates that I just saw seem wrong.

Hm, okay. Let me hold off on continuing then; I'm only about 25% in. The
general rule I was applying was "if you were marked Reviewer prior to
June, and you haven't interacted with the patchset this commitfest, I've
removed you."

>  The reviewers field was
> nullified, even though the patches haven't been updated in a long time.
> There's nothing new to review.  All this has done is lost information that
> someone else (me, in this case) went to the bother of adding.

My understanding from upthread was that we wanted to get out of the
habit of using Reviewers as a historical record, and move towards using
it as a marker of current activity. As Tom said, "people see that the
patch already has a reviewer and look for something else to do."

I am sorry that I ended up reverting your work, though.

> Also, cfapp has a page for "patches where you are the author", but the cfbot
> doesn't,

(I assume you mean "reviewer"?)

> and I think people probably look at cfbot more than the cfapp itself.

I think some people do. But the number of dead/non-applicable patches
that need manual reminders suggests to me that maybe it's not an
overwhelming majority of people.

> So being marked as a reviewer is not very visible even to oneself.
> But, one of the cfbot patches I sent to Thomas would change that.  Each user's
> page would *also* show patches where they're a reviewer ("Needs review -
> Reviewer").  That maybe provides an incentive to 1) help maintain the patch; or
> otherwise 2) remove oneself.
I didn't notice cfbot's user pages until this CF, so it wouldn't have
been an effective incentive for me, at least.

Also, I would like to see us fold cfbot output into the official CF,
rather than do the opposite.

> Also, TBH, this seems to create a lot of busywork.

Well, yes, but only because it's not automated. I don't think that's a
good reason not to do it, but it is a good reason not to make a person
do it.

>  I'd prefer to see someone
> pick one of the patches that hasn't seen a review in 6 (or 16) months, and send
> out their most critical review and recommend it be closed, or send an updated
> patch with their own fixes as an 0099 patch.

That would be cool, but who is "someone"? There have been many, many
statements about the amount of CF cruft that needs to be removed. Seems
like the CFM is in a decent position to actually do it.

--Jacob

On Fri, Jul 15, 2022 at 03:17:49PM -0700, Jacob Champion wrote:
> > Also, cfapp has a page for "patches where you are the author", but the cfbot
> > doesn't,
> 
> (I assume you mean "reviewer"?)

Yes

> Also, I would like to see us fold cfbot output into the official CF,
> rather than do the opposite.

That's been the plan for years :)

> > I'd prefer to see someone
> > pick one of the patches that hasn't seen a review in 6 (or 16) months, and send
> > out their most critical review and recommend it be closed, or send an updated
> > patch with their own fixes as an 0099 patch.
> 
> That would be cool, but who is "someone"? There have been many, many
> statements about the amount of CF cruft that needs to be removed. Seems
> like the CFM is in a decent position to actually do it.

I have hesitated to even try to begin the conversation.

Since a patch author initially creates the CF entry, why shouldn't they also be
responsible for moving them to the next cf.  This serves to indicate a
continued interest.  Ideally they also set back to "needs review" after
addressing feedback, but I imagine many would forget, and this seems like a
reasonable task for a CFM to do - look at WOA patches that pass tests to see if
they're actually WOA.

Similarly, patches could be summarily set to "waiting on author" if they didn't
recently apply, compile, and pass tests.  That's the minimum standard.
However, I think it's better not to do this immediately after the patch stops
applying/compiling/failing tests, since it's usually easy enough to review it.

It should be the author's responsibility to handle that; I don't know why the
accepted process seems to involve sending dozens of emails to say "needs
rebase".  You're putting to good use of some cfapp email features I don't
remember seeing used before; that seems much better.  Also, it's possible to
subscribe to CF updates (possibly not a well-advertised, well-known or
well-used feature).  

I didn't know until recently that when a CF entry is closed, that it's possible
(I think) for the author themselves to reopen it and "move it to the next CF".
I suggest to point this out to people; I suppose I'm not the only one who finds
it offputting when a patch is closed in batch at the end of the month after
getting only insignificant review.

Thanks for considering

-- 
Justin

On 7/15/22 16:15, Justin Pryzby wrote:
> On Fri, Jul 15, 2022 at 03:17:49PM -0700, Jacob Champion wrote:
>> Also, I would like to see us fold cfbot output into the official CF,
>> rather than do the opposite.
> 
> That's been the plan for years :)

Is there something other than lack of round tuits that's blocking
progress? I'm happy to donate more time in this area, but I don't know
if my first patch proposal was helpful (or even on the right list --
pgsql-www, right?).

>>> I'd prefer to see someone
>>> pick one of the patches that hasn't seen a review in 6 (or 16) months, and send
>>> out their most critical review and recommend it be closed, or send an updated
>>> patch with their own fixes as an 0099 patch.
>>
>> That would be cool, but who is "someone"? There have been many, many
>> statements about the amount of CF cruft that needs to be removed. Seems
>> like the CFM is in a decent position to actually do it.
> 
> I have hesitated to even try to begin the conversation.
> 
> Since a patch author initially creates the CF entry, why shouldn't they also be
> responsible for moving them to the next cf.  This serves to indicate a
> continued interest.  Ideally they also set back to "needs review" after
> addressing feedback, but I imagine many would forget, and this seems like a
> reasonable task for a CFM to do - look at WOA patches that pass tests to see if
> they're actually WOA.

I agree in principle -- I think, ideally, WoA patches should be
procedurally closed at the end of a commitfest, and carried forward when
the author has actually responded. The problems I can imagine resulting
from this are

- Some reviewers mark WoA _immediately_ upon sending an email. I think
authors should have a small grace period to respond before having their
patches automatically "muted" by the system, if the review happens right
at the end of the CF.

- Carrying forward a closed patch is not actually easy. See below.

> Similarly, patches could be summarily set to "waiting on author" if they didn't
> recently apply, compile, and pass tests.  That's the minimum standard.
> However, I think it's better not to do this immediately after the patch stops
> applying/compiling/failing tests, since it's usually easy enough to review it.

It's hard to argue with that, but without automation, this is plenty of
busy work too.

> It should be the author's responsibility to handle that; I don't know why the
> accepted process seems to involve sending dozens of emails to say "needs
> rebase".  You're putting to good use of some cfapp email features I don't
> remember seeing used before; that seems much better.  Also, it's possible to
> subscribe to CF updates (possibly not a well-advertised, well-known or
> well-used feature).  

I don't think it should be reviewers' responsibility to say "needs
rebase" over and over again, and I also don't think a new author should
have to refresh the cfbot every single day to find out whether or not
their patch still applies. These things should be handled by the app.

(Small soapbox, hopefully relevant: I used to be in the camp that making
contributors jump through small procedural hoops would somehow weed out
people who were making low-effort patches. I've since come around to the
position that this just tends to select for people with more free time
and/or persistence. I don't like the idea of raising the busy-work bar
for authors, especially without first fixing the automation problem.)

> I didn't know until recently that when a CF entry is closed, that it's possible
> (I think) for the author themselves to reopen it and "move it to the next CF".
> I suggest to point this out to people; I suppose I'm not the only one who finds
> it offputting when a patch is closed in batch at the end of the month after
> getting only insignificant review.

I think this may have been the goal but I don't think it actually works
in practice. The app refuses to let you carry a RwF patch to a new CF.

--

This is important stuff to discuss, for sure, but I also want to revisit
the thing I put on pause, which is to clear out old Reviewer entries to
make it easier for new reviewers to find work to do. If we're not using
Reviewers as a historical record, is there any reason for me not to keep
clearing that out?

It undoes work that you and others have done to make the historical
record more accurate, and I think that's understandably frustrating. But
I thought we were trying to move away from that usage of it altogether.

--Jacob

On Fri, Jul 15, 2022 at 05:23:48PM -0700, Jacob Champion wrote:
> On 7/15/22 16:15, Justin Pryzby wrote:
> > On Fri, Jul 15, 2022 at 03:17:49PM -0700, Jacob Champion wrote:
> >> Also, I would like to see us fold cfbot output into the official CF,
> >> rather than do the opposite.
> > 
> > That's been the plan for years :)
> 
> Is there something other than lack of round tuits that's blocking
> progress? I'm happy to donate more time in this area, but I don't know
> if my first patch proposal was helpful (or even on the right list --
> pgsql-www, right?).

cfbot is Thomas's project, so moving it run on postgres vm was one step, but I
imagine the "integration with cfapp" requires coordination with Magnus.

What patch ?

> > Similarly, patches could be summarily set to "waiting on author" if they didn't
> > recently apply, compile, and pass tests.  That's the minimum standard.
> > However, I think it's better not to do this immediately after the patch stops
> > applying/compiling/failing tests, since it's usually easy enough to review it.
> 
> It's hard to argue with that, but without automation, this is plenty of
> busy work too.

I don't think that's busywork, since it's understood to require human
judgement, like 1) to deal with false-positive test failures, and 2) check if
there's actually anything left for the author to do; 3) check if it passed
tests recently; 4) evaluate existing opinions in the thread and make a
judgement call.

> > I didn't know until recently that when a CF entry is closed, that it's possible
> > (I think) for the author themselves to reopen it and "move it to the next CF".
> > I suggest to point this out to people; I suppose I'm not the only one who finds
> > it offputting when a patch is closed in batch at the end of the month after
> > getting only insignificant review.
> 
> I think this may have been the goal but I don't think it actually works
> in practice. The app refuses to let you carry a RwF patch to a new CF.

I was able to do what Peter said.  I don't know why the cfapp has that
restriction, it seems like an artificial constraint.

https://www.postgresql.org/message-id/8498f959-e7a5-b0ec-7761-26984e581a51%40enterprisedb.com
https://commitfest.postgresql.org/32/2888/

-- 
Justin

On Fri, Jul 15, 2022 at 05:23:48PM -0700, Jacob Champion wrote:
> This is important stuff to discuss, for sure, but I also want to revisit
> the thing I put on pause, which is to clear out old Reviewer entries to
> make it easier for new reviewers to find work to do. If we're not using
> Reviewers as a historical record, is there any reason for me not to keep
> clearing that out?

> It undoes work that you and others have done to make the historical
> record more accurate, and I think that's understandably frustrating. But
> I thought we were trying to move away from that usage of it altogether.

I don't agree that I'm using it "for historical record".  See 3499.

There's certainly some value in updating the cfapp to be "more accurate" for
some definition.  By chance, I saw the "activity log".
https://commitfest.postgresql.org/activity/

Honestly, most of the changes seems to be for the worse (16 patches had the
review field nullified).  Incomplete list of changes:

3609 - removed Nathan
3561 - removed Michael
3046 - removed PeterE
3396 - removed Tom
3396 - removed Robert and Bharath
2710 - removed Julien
3612 - removed Nathan (added by Greg)
3295 - removed Andrew
2573 - removed Daniel
3623 - removed Hou Zhijie
3260 - removed Fabien
3041 - removed Masahiko
2161 - removed Michael

I'm not suggesting to give the community regulars special treatment, but you
could reasonably assume that when they added themselves and then "didn't remove
themself", it was on purpose and not by omission.  I think most of those people
would be surprised to learn that they're no longer considered to be reviewing
the patch.

> If someone put a lot of review into a patchset a few months ago, they
> absolutely deserve credit. But if that entry has been sitting with no
> feedback this month, why is it useful to keep that Reviewer around?

I don't know what to say to this.

Why do you think it's useful to remove annotations that people added ? (And, if
it were useful, why shouldn't that be implemented in the cfapp, which already
has all the needed information.)

Can you give an example of a patch where you sent a significant review, and
added yourself as a reviewer, but wouldn't mind if someone summarily removed
you, in batch ?  It seems impolite to remove someone who is, in fact, a
reviewer.

The stated goal was to avoid the scenario that a would-be reviewer decides not
to review a patch because cfapp already shows someone else as a reviewer.  I'm
sure that could happen, but I doubt it's something that happens frequently..

-- 
Justin

On Fri, Jul 15, 2022 at 05:23:48PM -0700, Jacob Champion wrote:
> I agree in principle -- I think, ideally, WoA patches should be
> procedurally closed at the end of a commitfest, and carried forward when
> the author has actually responded. The problems I can imagine resulting
> from this are
>
> - Some reviewers mark WoA _immediately_ upon sending an email. I think
> authors should have a small grace period to respond before having their
> patches automatically "muted" by the system, if the review happens right
> at the end of the CF.

On this point, I'd like to think that a window of two weeks is a right
balance.  That's half of the commit fest, so that leaves plenty of
time for one to answer.  There is always the case where one is on
vacations for a period longer than that, but it is also possible for
an author to add a new entry in a future CF for the same patch.  If I
recall correctly, it should be possible to move a patch that has been
closed even if the past CF has been marked as been concluded, allowing
one to keep the same patch with its history and annotations.
--
Michael

On Fri, Jul 15, 2022 at 09:37:14PM -0500, Justin Pryzby wrote:
> I'm not suggesting to give the community regulars special treatment, but you
> could reasonably assume that when they added themselves and then "didn't remove
> themself", it was on purpose and not by omission.  I think most of those people
> would be surprised to learn that they're no longer considered to be reviewing
> the patch.

Yeah, I happened to look in my commitfest update folder this morning and
was surprised to learn that I was no longer reviewing 3612.  I spent a good
amount of time getting that patch into a state where I felt it was
ready-for-committer, and I intended to follow up on any further
developments in the thread.  It's not uncommon for me to use the filter
functionality in the app to keep track of patches I'm reviewing.

Of course, there are probably patches where I could be removed from the
reviewers field.  I can try to stay on top of that better.  If you think I
shouldn't be marked as a reviewer and that it's hindering further review
for a patch, feel free to message me publicly or privately about it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Maybe we should have two reviewers columns -- one for history-tracking
purposes (and commit msg credit) and another for current ones.

Personally, I don't use the CF app when building reviewer lists.  I scan
the threads instead.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Mon, Jul 18, 2022 at 03:05:51PM +0200, Alvaro Herrera wrote:
> Maybe we should have two reviewers columns -- one for history-tracking
> purposes (and commit msg credit) and another for current ones.

Maybe.  Or, the list of reviewers shouldn't be shown prominently in the list of
patches.  But changing that would currently break cfbot's web scraping.

-- 
Justin

On Fri, Jul 15, 2022 at 05:23:48PM -0700, Jacob Champion wrote:
>> This is important stuff to discuss, for sure, but I also want to revisit
>> the thing I put on pause, which is to clear out old Reviewer entries to
>> make it easier for new reviewers to find work to do. If we're not using
>> Reviewers as a historical record, is there any reason for me not to keep
>> clearing that out?

On Fri, Jul 15, 2022 at 09:37:14PM -0500, Justin Pryzby wrote:
> Why do you think it's useful to remove annotations that people added ? (And, if
> it were useful, why shouldn't that be implemented in the cfapp, which already
> has all the needed information.)

Or, to say it differently, since "reviewers" are preserved when a patch is
moved to the next CF, it comes as a surprise when by some other mechanism or
policy the field doesn't stay there.  (If it's intended to be more like a
per-CF field, I think its behavior should be changed in the cfapp, to avoid
manual effort, and to avoid other people executing it differently.)

> It undoes work that you and others have done to make the historical
> record more accurate, and I think that's understandably frustrating. But
> I thought we were trying to move away from that usage of it altogether.

I gather that your goal was to make the "reviewers" field more like "people who
are reviewing the current version of the patch", to make it easy to
find/isolate patch-versions which need to be reviewed, and hopefully accelarate
the process.

But IMO there's already no trouble finding the list of patches which need to be
reviewed - it's the long list that say "Needs Review" - which is what's
actually needed; that's not easy to do, which is why it's a long list, and no
amount of updating the annotations will help with that.  I doubt many people
search for patches to review by seeking out those which have no reviewer (which
is not a short list anyway).  I think they look for the most interesting
patches, or the ones that are going to be specifically useful to them.

Here's an idea: send out batch mails to people who are listed as reviewers for
patches which "Need Review".  That starts to treat the reviewers field as a
functional thing rather than purely an annotation.  Be sure in your message to
say "You are receiving this message because you're listed as a reviewer for a
patch which -Needs Review-".  I think it's reasonable to get a message like
that 1 or 2 times per month (but not per-month-per-patch).  Ideally it'd
include the list of patches specific to that reviewer, but I think it'd okay to
get an un-personalized email reminder 1x/month with a link.

BTW, one bulk update to make is for the few dozen patches that say "v15" on
them, and (excluding bugfixes) those are nearly all wrong.  Since the field
isn't visible in cfbot, it's mostly ignored.  The field is useful toward the
end of a release cycle to indicate patches that aren't intended for
consideration for the next release.  Ideally, it'd also be used to indicate the
patches *are* being considered, but it seems like nobody does that and it ends
up being a surprise which patches are or are not committed (this seems weird
and easy to avoid but .. ).  The patches which say "v15" are probably from
patches submitted during the v15 cycle, and now the version should be removed,
unless there's a reason to believe the patch is going to target v16 (like if a
committer has assigned themself).

Thanks for receiving my criticism well :)

-- 
Justin

Justin,

(Consolidating replies here.)

On 7/15/22 19:13, Justin Pryzby wrote:
> cfbot is Thomas's project, so moving it run on postgres vm was one step, but I
> imagine the "integration with cfapp" requires coordination with Magnus.
> 
> What patch ?

https://www.postgresql.org/message-id/CAAWbhmg84OsO5VkaSjX4jokHy8mdpWpNKFgZJHHbb4mprXmtiQ%40mail.gmail.com

It was intended to be a toe in the water -- see if I'm following
conventions, and if I even have the right list.

>>> Similarly, patches could be summarily set to "waiting on author" if they didn't
>>> recently apply, compile, and pass tests.  That's the minimum standard.
>>> However, I think it's better not to do this immediately after the patch stops
>>> applying/compiling/failing tests, since it's usually easy enough to review it.
>>
>> It's hard to argue with that, but without automation, this is plenty of
>> busy work too.
> 
> I don't think that's busywork, since it's understood to require human
> judgement, like 1) to deal with false-positive test failures, and 2) check if
> there's actually anything left for the author to do; 3) check if it passed
> tests recently; 4) evaluate existing opinions in the thread and make a
> judgement call.

[Dev hat; strong opinions ahead.]

I maintain that 1) and 3) are busy work. You should not have to do those
things, in the ideal end state.

>> I think this may have been the goal but I don't think it actually works
>> in practice. The app refuses to let you carry a RwF patch to a new CF.
> 
> I was able to do what Peter said.  I don't know why the cfapp has that
> restriction, it seems like an artificial constraint.
Thanks, I'll work on a patch.

> On Fri, Jul 15, 2022 at 05:23:48PM -0700, Jacob Champion wrote:
> I'm not suggesting to give the community regulars special treatment, but you
> could reasonably assume that when they added themselves and then "didn't remove
> themself", it was on purpose and not by omission.  I think most of those people
> would be surprised to learn that they're no longer considered to be reviewing
> the patch.

For some people, I can maybe(?) assume that, but I'm being honest when I
say that I don't really know who that's true for. I definitely don't
think it's true for everybody. And once I start making those decisions
as a CFM, then it really does boil down to who I know and have
interacted with before.

> Can you give an example of a patch where you sent a significant review, and
> added yourself as a reviewer, but wouldn't mind if someone summarily removed
> you, in batch ?

Literally all of them. That's probably the key disconnect here, and why
I didn't think too hard about doing it. (I promise, I didn't think to
myself "I would really hate it if someone did this to me", and then go
ahead and do it to twenty-some other people. :D)

I come from OSS communities that discourage cookie-licking, whether
accidental or on purpose. I don't like marking myself as a Reviewer in
general (although I have done it, because it seems like the thing to do
here?). Simultaneous reviews are never "wasted work" and I'd just rather
not call dibs on a patch. So I wouldn't have a problem with someone
coming along, seeing that I haven't interacted with a patch for a while,
and removing my name. I trust that committers will give credit if credit
is due.

> The stated goal was to avoid the scenario that a would-be reviewer decides not
> to review a patch because cfapp already shows someone else as a reviewer.  I'm
> sure that could happen, but I doubt it's something that happens frequently..

I do that every commitfest. It's one of the first things I look at to
determine what to pay attention to, because I'm trying to find patches
that have slipped through the cracks. As Tom pointed out, others do it
too, though I don't know how many or if their motivations match mine.

>> Why do you think it's useful to remove annotations that people added ? (And, if
>> it were useful, why shouldn't that be implemented in the cfapp, which already
>> has all the needed information.)
> 
> Or, to say it differently, since "reviewers" are preserved when a patch is
> moved to the next CF, it comes as a surprise when by some other mechanism or
> policy the field doesn't stay there.  (If it's intended to be more like a
> per-CF field, I think its behavior should be changed in the cfapp, to avoid
> manual effort, and to avoid other people executing it differently.)

It was my assumption, based on the upthread discussion, that that was
the end goal, and that we just hadn't implemented it yet for lack of time.

>> It undoes work that you and others have done to make the historical
>> record more accurate, and I think that's understandably frustrating. But
>> I thought we were trying to move away from that usage of it altogether.
> 
> I gather that your goal was to make the "reviewers" field more like "people who
> are reviewing the current version of the patch", to make it easy to
> find/isolate patch-versions which need to be reviewed, and hopefully accelarate
> the process.

Yes.
> But IMO there's already no trouble finding the list of patches which need to be
> reviewed - it's the long list that say "Needs Review" - which is what's
> actually needed; that's not easy to do, which is why it's a long list, and no
> amount of updating the annotations will help with that.  I doubt many people
> search for patches to review by seeking out those which have no reviewer (which
> is not a short list anyway).

I do, because I'm looking to maximize bang for buck. When people have
asked me as CFM for help finding patches to review, that's one of the
criteria I use. Needs Review is just too broad, and frankly seems to be
overwhelming to new reviewers I've spoken with.

Honestly, what I really want is buckets based on categories of *what you
can actually do for the patch*. "We want this but it's been stuck in
perma-rebase", "this is blocked on something you can't possibly
influence as a reviewer", "this needs review from someone who
understands every internal system", "this has been stale for months and
is on the chopping block". These are all things that seem to pop up
semi-regularly.

> Here's an idea: send out batch mails to people who are listed as reviewers for
> patches which "Need Review".  That starts to treat the reviewers field as a
> functional thing rather than purely an annotation.  Be sure in your message to
> say "You are receiving this message because you're listed as a reviewer for a
> patch which -Needs Review-".  I think it's reasonable to get a message like
> that 1 or 2 times per month (but not per-month-per-patch).  Ideally it'd
> include the list of patches specific to that reviewer, but I think it'd okay to
> get an un-personalized email reminder 1x/month with a link.

I don't think that addresses the goal, or at least not the goal that I
was pursuing when I was pruning this field. Reminding existing reviewers
doesn't help organize the incoming funnel of new reviewers. And while it
might remind the regular contributors to remove their names if they've
gone stale, it won't ease the CFM busywork for people who have
completely gone silent.

> BTW, one bulk update to make is for the few dozen patches that say "v15" on
> them, and (excluding bugfixes) those are nearly all wrong.  Since the field
> isn't visible in cfbot, it's mostly ignored.  The field is useful toward the
> end of a release cycle to indicate patches that aren't intended for
> consideration for the next release.  Ideally, it'd also be used to indicate the
> patches *are* being considered, but it seems like nobody does that and it ends
> up being a surprise which patches are or are not committed (this seems weird
> and easy to avoid but .. ).  The patches which say "v15" are probably from
> patches submitted during the v15 cycle, and now the version should be removed,
> unless there's a reason to believe the patch is going to target v16 (like if a
> committer has assigned themself).

My recent track record makes me reluctant to do any more bulk updates if
I don't really understand how a field is being used. That whole thing
sounds like something we should address with a better workflow.

> Thanks for receiving my criticism well :)

And thank you for speaking up so quickly! It's a lot easier to undo
partial damage :D (Speaking of which: where is that CF update stream you
mentioned?)

--Jacob

On 7/18/22 06:13, Justin Pryzby wrote:
> On Mon, Jul 18, 2022 at 03:05:51PM +0200, Alvaro Herrera wrote:
>> Maybe we should have two reviewers columns -- one for history-tracking
>> purposes (and commit msg credit) and another for current ones.
> 
> Maybe.  Or, the list of reviewers shouldn't be shown prominently in the list of
> patches.  But changing that would currently break cfbot's web scraping.

I think separating use cases of "what you can currently do for this
patch" and "what others have historically done for this patch" is
important. Whether that's best done with more columns or with some other
workflow, I'm not sure.

It seems like being able to mark items on a personal level, in a way
that doesn't interfere with recordkeeping being done centrally, could
help as well.

--Jacob

On 7/15/22 19:59, Michael Paquier wrote:
> On this point, I'd like to think that a window of two weeks is a right
> balance.  That's half of the commit fest, so that leaves plenty of
> time for one to answer.  There is always the case where one is on
> vacations for a period longer than that, but it is also possible for
> an author to add a new entry in a future CF for the same patch.

That seems reasonable. My suggestion was going to be more aggressive, at
five days, but really anywhere in that range seems good.

--Jacob

On 7/17/22 08:17, Nathan Bossart wrote:
> On Fri, Jul 15, 2022 at 09:37:14PM -0500, Justin Pryzby wrote:
>> I'm not suggesting to give the community regulars special treatment, but you
>> could reasonably assume that when they added themselves and then "didn't remove
>> themself", it was on purpose and not by omission.  I think most of those people
>> would be surprised to learn that they're no longer considered to be reviewing
>> the patch.
> 
> Yeah, I happened to look in my commitfest update folder this morning and
> was surprised to learn that I was no longer reviewing 3612.  I spent a good
> amount of time getting that patch into a state where I felt it was
> ready-for-committer, and I intended to follow up on any further
> developments in the thread.  It's not uncommon for me to use the filter
> functionality in the app to keep track of patches I'm reviewing.

I'm sorry again for interrupting that flow. Thank you for speaking up
and establishing the use case.

> Of course, there are probably patches where I could be removed from the
> reviewers field.  I can try to stay on top of that better.  If you think I
> shouldn't be marked as a reviewer and that it's hindering further review
> for a patch, feel free to message me publicly or privately about it.

Sure. I don't plan on removing anyone else from a Reviewer list this
commitfest, but if I do come across a reason I'll make sure to ask first. :)

--Jacob

On Mon, Jul 18, 2022 at 12:06:34PM -0700, Jacob Champion wrote:
> On 7/17/22 08:17, Nathan Bossart wrote:
>> Yeah, I happened to look in my commitfest update folder this morning and
>> was surprised to learn that I was no longer reviewing 3612.  I spent a good
>> amount of time getting that patch into a state where I felt it was
>> ready-for-committer, and I intended to follow up on any further
>> developments in the thread.  It's not uncommon for me to use the filter
>> functionality in the app to keep track of patches I'm reviewing.
> 
> I'm sorry again for interrupting that flow. Thank you for speaking up
> and establishing the use case.

No worries.  Thanks for managing this commitfest!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Jul 18, 2022 at 12:00:01PM -0700, Jacob Champion wrote:
> And thank you for speaking up so quickly! It's a lot easier to undo
> partial damage :D (Speaking of which: where is that CF update stream you
> mentioned?)

https://commitfest.postgresql.org/activity/

-- 
Justin

On 7/18/22 15:32, Justin Pryzby wrote:
> On Mon, Jul 18, 2022 at 12:00:01PM -0700, Jacob Champion wrote:
>> And thank you for speaking up so quickly! It's a lot easier to undo
>> partial damage :D (Speaking of which: where is that CF update stream you
>> mentioned?)
> 
> https://commitfest.postgresql.org/activity/

Thank you. At this point, I think I've repaired all the entries.

--Jacob