On Mon, Apr 7, 2014 at 1:58 PM, Stephen Frost <sfr...@snowman.net> wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> If it's only going to take you an hour to address this patch (or 8 to >> address those other ones) then you spend a heck of a lot less time on >> review for a patch of a given complexity level than I do. > > Eh, I don't really time it and I'm probably completely off in reality, > it's more of a relative "feeling" thing wrt how long it'd take. I was > also thinking about it from a 'initial review and provide feedback' > standpoint, actually getting to a point of committing something > certainly takes much longer, but I can also kick off tests and do > individual testing in those smaller time blocks. Reading the code and > understanding it, writing up the feedback email, etc, is what requires > the larger single block. Perhaps I can work on improving that for > myself and maybe find a way to do it in smaller chunks, but that hasn't > happened in the however-many-years, and there's the whole 'old dog, new > tricks' issue.
It takes me a big block of time, too, at least for the initial review. > The issue on it being called "poorman"? That doesn't exactly strike me > as needing a particularly long discussion, nor that it would be > difficult to change later. I agree that there may be other issues, and > it'd be great to get buy-in from everyone before anything goes in, but > there's really zero hope of that being a reality. Really? I think there have been just about zero patches that have gone in in the last (ugh) three months that have had significant unaddressed objections from anyone at the time they were committed. There has certainly been an enormous amount of work by a whole lot of people to address objections that have been raised, and generally that has gone well. I will not pretend that every patch that has gone in is completely well-liked by everyone and I am sure that is not the case. Nevertheless I think we've done pretty well. Now the people whose stuff has not got in - and may not get in - may well feel that their stuff got short shrift, and I'm not going to deny that there's a problem there. But breaking from our usual procedure for this patch is just adding to that unfairness, not making anything better. >> And that discussion will require the time not only of the people who >> find this patch more interesting than any other, but also of the >> people who just said that they're busy with other things right now, >> unless those people want to forfeit their right to an opinion. > > I certainly hope that no committer feels that they forfeit their right > to an opinion about a piece of code because they didn't object to it > before it was committed. My experience is that committed code gets > reviewed and concerns are raised, at which point it's usually on the > original committer to go back and fix it; which I'm certainly glad for. Sure, people can object to anything whenever they like. But it becomes an uphill battle once it goes in, unless the breakage is rather flagrant. Regardless, if this patch had had multiple, detailed reviews finding lots of issues that were then addressed, I'd probably be keeping my trap shut and hoping for the best. But it hasn't. Any patch of this size is going to have nitpicky stuff that needs to be addressed, if nothing else, and points requiring some modicum of public discussion, and there hasn't been any of that. That suggests to me that it just hasn't been thoroughly reviewed yet, at least not in public, and that should happen long before anyone talks about picking it up for commit. If this patch had been timely submitted and I'd picked it up right away, I would have expected at least three weeks to elapse between the time my first review was posted and the time all the details were nailed down, even assuming no more serious problems were found, and starting that ball rolling now means looking forward to a commit around the end of April assuming things go great. That seems way too late to me; IMHO, we should already be mostly in mop-up mode now (hence my recent DSM patch cleanup patch, which no one has commented on...). And I think it's likely that, in fact, we'll find cases where this regresses performance rather badly, for reasons sketched in an email I posted a bit ago. -- 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
