Re: B-Tree support function number 3 (strxfrm() optimization) - Mailing list pgsql-hackers

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

That implies that everyone has been reviewing everything, which is
certainly not entirely the case..  If that's happening then I'm not sure
I understand the point of the commitfest, and feel I must be falling
down on the job here.  Certainly there are patches which have been
picked up by committers and committed that I've not reviewed.  I also
didn't object, but if I find issue with them in the future, I'd
certainly bring them up, even post-commit.

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

This goes back to the point which I was trying to make earlier-
prioritization.  Small+clearly-good patches should be prioritized
higher, but identifying those is no small matter.  Of course, we would
need to address the starvation risk when it comes to larger patches,
which I've got no answer for.

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

Perhaps my memory is foggy, but I recall at least some amount of
discussion and review of this, along with fixes going in, over the past
couple of weeks.  More may be needed, of course, but if so, I'd expect
any committer looking at it would realize that and bounce it at this
point, given the feature freeze deadline which is like next week..

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

I agree with this.

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

I've not researched it enough myself to say.
Thanks,
    Stephen