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

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

> I agree
> that it's desirable to slip things in, from time to time, when they're
> uncontroversial and obviously meritorious, but I'm not completely
> convinced that this is such a case.  As an utterly trivial point, I
> find the naming to be less than ideal: "poorman" is not a term I want
> to enshrine in our code.  That's not very descriptive of what the
> patch is actually doing even if you know what the idiom means, and
> people whose first language - many of whom do significant work on our
> code - may not.

Fair enough.

> Now the point is not that that's a serious flaw in and of itself.  The
> point is that these kinds of issues deserve to be discussed and agreed
> on, and the process should be structured in a way that permits that.

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.

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

> Experience has shown that it's a whole lot easier for anyone here to
> get a patch changed before it's committed than after it's committed,
> so I don't buy your argument that the timing there doesn't matter.

Once code has been released and there are external dependencies on it,
that's obviously an issue.  Ahead of that, I feel like the issue is
really more one of interest- everyone is very interested when code is
about to go in, but once it's in, for whatever reason, the interest
becomes much less to review and comment on it.  I feel like that's a
very long-standing issue as it relates to our 'beta' period because
doing code review just simply isn't fun and the motivation is reduced
once it's been committed.

That makes me wonder about having "beta" review-fests (in fact, I feel
like that may have been proposed before...), where commits are assigned
out to be reviewed by someone other than the committer/author/original
reviewer, as a way to motivate individuals to go review what has gone
in before we get to release.  That might help us ensure that more of the
committed code *gets* another review before release and reduce the
issues we have post-release.  Just a thought.
Thanks,
    Stephen