Re: B-Tree support function number 3 (strxfrm() optimization)

On Mon, Apr 7, 2014 at 1:58 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@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
pickedit 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