fun with "Ready for Committer" patches

Started by Robert Haasalmost 10 years ago11 messages
#1Robert Haas
robertmhaas@gmail.com

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Joe Conway
mail@joeconway.com
In reply to: Robert Haas (#1)
Re: fun with "Ready for Committer" patches

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

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#1)
Re: fun with "Ready for Committer" patches

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Tatsuo Ishii
ishii@postgresql.org
In reply to: Robert Haas (#1)
Re: fun with "Ready for Committer" patches

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Craig Ringer
craig@2ndquadrant.com
In reply to: Tatsuo Ishii (#4)
Re: fun with "Ready for Committer" patches

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

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

#6Andres Freund
andres@anarazel.de
In reply to: Tatsuo Ishii (#4)
Re: fun with "Ready for Committer" patches

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Robert Haas
robertmhaas@gmail.com
In reply to: Tatsuo Ishii (#4)
Re: fun with "Ready for Committer" patches

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Robert Haas
robertmhaas@gmail.com
In reply to: Craig Ringer (#5)
Re: fun with "Ready for Committer" patches

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#9Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#8)
Re: fun with "Ready for Committer" patches

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Andres Freund
andres@anarazel.de
In reply to: Michael Paquier (#9)
Re: fun with "Ready for Committer" patches

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Michael Paquier
michael.paquier@gmail.com
In reply to: Andres Freund (#10)
Re: fun with "Ready for Committer" patches

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers