Thread: fun with "Ready for Committer" patches
OK, so I made a pass through the "Ready for Committer" patches in the current CF. One I committed, several I replied to the thread with review comments and set back to "Waiting on Author". Here's where we are with the rest: Silent data loss with ext4 / all current versions - It looks to me like Andres is handling this. I set him as the committer. pg_resetxlog reference page reorganization - Peter Eisentraut wrote this patch. I assume he will commit it. If not, I'm not sure why anyone else should. GCC 6 warning fixes - Ditto. TAP test enhancements - It looks to me like Alvaro is handling this. I set him as the committer. Unique Joins - This patch has had a lot of review and discussion. It would be best if Tom Lane looked at it. If not, one of us lesser mortals will have to have a go. index-only scans with partial indexes - Kevin has claimed this as committer. Fix lock contention for HASHHDR.mutex - I guess I need to go revisit this one, unless somebody else is willing to jump in. I wouldn't mind a few more opinions on this patch. plpgsql - possibility to get element or array type for referenced variable type - No committer yet. I don't have enough interest or knowledge to want to handle this. pl/pgSQL, get diagnostics and big data - No committer yet. Seems like a reasonable concept. A borderline bug fix. enhanced custom error in PLPythonu - No committer yet. Tom Lane and Peter Eisentraut are the usual suspects for PL/python. Again, I have neither the interest nor the knowledge. It's hard to miss the fact that there are an absolutely breathtaking number of patches in this CommitFest - 80! - that are in the "needs review" state. We really, really, really need more review to happen - and there's no place for that to come from except other people who would like their own patches reviewed in turn, other than the limited bandwidth of committers and of course the absolutely stunning efforts of Michael Paquier to review everything in sight. But that's not going to be enough: we really, really need other people to be reviewing also. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 03/08/2016 02:45 PM, Robert Haas wrote: > OK, so I made a pass through the "Ready for Committer" patches in the > current CF. One I committed, several I replied to the thread with > review comments and set back to "Waiting on Author". Here's where we > are with the rest: > plpgsql - possibility to get element or array type for referenced > variable type - No committer yet. I don't have enough interest or > knowledge to want to handle this. I'll try to move this one forward, although later in the week as I am traveling all day tomorrow > pl/pgSQL, get diagnostics and big data - No committer yet. Seems like > a reasonable concept. A borderline bug fix. possibly this one too... Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Robert Haas <robertmhaas@gmail.com> writes:
> Unique Joins - This patch has had a lot of review and discussion. It
> would be best if Tom Lane looked at it.
Yeah, I'll pick it up soon. I've basically been kicking as much as
I could down the road for the last couple of months, trying to get the
pathification changes done. Now that that's in, I expect to be
substantially less AWOL from the commitfest.
> enhanced custom error in PLPythonu - No committer yet. Tom Lane and
> Peter Eisentraut are the usual suspects for PL/python. Again, I have
> neither the interest nor the knowledge.
I don't mind touching plpython at the C level, but this one requires
somebody who uses Python enough to have an informed opinion on the
tastefulness of the proposed language features. That's not me.
regards, tom lane
> It's hard to miss the fact that there are an absolutely breathtaking > number of patches in this CommitFest - 80! - that are in the "needs > review" state. We really, really, really need more review to happen - Many of "needs review" state patches already have reviewer(s). Do you mean we want more reviewers in addition to them for such patches? Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On 9 March 2016 at 07:18, Tatsuo Ishii <ishii@postgresql.org> wrote:
Many of "needs review" state patches already have reviewer(s). Do you
mean we want more reviewers in addition to them for such patches?
Yeah. Personally I'm not too confident about what precisely is required to move a patch from needs-review to ready-for-committer. I've done a chunk of review for a number of patches, but I'm not always confident saying "all clear, proceed".
On 2016-03-09 08:18:09 +0900, Tatsuo Ishii wrote: > > It's hard to miss the fact that there are an absolutely breathtaking > > number of patches in this CommitFest - 80! - that are in the "needs > > review" state. We really, really, really need more review to happen - > > Many of "needs review" state patches already have reviewer(s). Do you > mean we want more reviewers in addition to them for such patches? Any review performed is worthwhile. Somebody having signed up to review a patch shouldn't stop you. Especially if that signup was more than a couple hours ago. Andres
On Tue, Mar 8, 2016 at 6:18 PM, Tatsuo Ishii <ishii@postgresql.org> wrote: >> It's hard to miss the fact that there are an absolutely breathtaking >> number of patches in this CommitFest - 80! - that are in the "needs >> review" state. We really, really, really need more review to happen - > > Many of "needs review" state patches already have reviewer(s). Do you > mean we want more reviewers in addition to them for such patches? Well, what we mostly need is more *reviews*. Whether those are from the reviewers already signed up or new reviewers is, IMHO, a secondary consideration. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Mar 8, 2016 at 9:44 PM, Craig Ringer <craig@2ndquadrant.com> wrote: > On 9 March 2016 at 07:18, Tatsuo Ishii <ishii@postgresql.org> wrote: >> Many of "needs review" state patches already have reviewer(s). Do you >> mean we want more reviewers in addition to them for such patches? > > Yeah. Personally I'm not too confident about what precisely is required to > move a patch from needs-review to ready-for-committer. I've done a chunk of > review for a number of patches, but I'm not always confident saying "all > clear, proceed". I think that if you've done your best to review it, and you don't see any remaining problems, you should mark it Ready for Committer. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 9, 2016 at 9:47 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Mar 8, 2016 at 9:44 PM, Craig Ringer <craig@2ndquadrant.com> wrote: >> On 9 March 2016 at 07:18, Tatsuo Ishii <ishii@postgresql.org> wrote: >>> Many of "needs review" state patches already have reviewer(s). Do you >>> mean we want more reviewers in addition to them for such patches? >> >> Yeah. Personally I'm not too confident about what precisely is required to >> move a patch from needs-review to ready-for-committer. I've done a chunk of >> review for a number of patches, but I'm not always confident saying "all >> clear, proceed". > > I think that if you've done your best to review it, and you don't see > any remaining problems, you should mark it Ready for Committer. IMO, during a review one needs to think of himself as a committer. Once the reviewer switches the patch to "Ready for committer", it means that the last version of the patch present would have been the version that gained the right to be pushed. -- Michael
On 2016-03-10 06:14:25 +0900, Michael Paquier wrote: > IMO, during a review one needs to think of himself as a committer. > Once the reviewer switches the patch to "Ready for committer", it > means that the last version of the patch present would have been the > version that gained the right to be pushed. And one consideration there is whether you, as the committer, would be ok with maintaining this feature going forward. But I think for less experienced reviewers that's hard to judge; and I think asking everyone to do that raises the barriers to do reviews considerably. So I think we should somehow document that it's ok to mark the patch as such, but that you're not forced to do that if you don't want to. Andres
On Wed, Mar 9, 2016 at 10:19 PM, Andres Freund <andres@anarazel.de> wrote: > On 2016-03-10 06:14:25 +0900, Michael Paquier wrote: >> IMO, during a review one needs to think of himself as a committer. >> Once the reviewer switches the patch to "Ready for committer", it >> means that the last version of the patch present would have been the >> version that gained the right to be pushed. > > And one consideration there is whether you, as the committer, would be > ok with maintaining this feature going forward. Yes, that's a key point. Committers do the initial push and the after-sales service as well. -- Michael