Re: More work on SortSupport for text - strcoll() and strxfrm() caching

On Mon, Oct 12, 2015 at 3:31 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Oct 12, 2015 at 12:47 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>> I also noticed that I failed to reset the last_returned strcoll()
>> cache variable as part of an abbreviation call, despite the fact that
>> tapesort may freely interleave conversions with comparisons, while
>> reusing buf1 and buf2 both as scratch space for strxfrm() blobs, as
>> well as for storing strings to be compared with strcoll(). I suggest
>> that the attached patch also be applied to fix this issue.
>
> I think that I jumped the gun with this fix, because theoretically you
> can still get the same problem in the opposite direction -- an
> original string treated as a strxfrm() blob when the cache is
> consulted.
>
> I'll consider a more comprehensive fix.

This is not the first time I've committed one of your patches and
promptly received a whole series of emails with fixes for what went
into that commit, despite the fact that I have not and did not change
the relevant parts of the patch while committing. Since that makes
more work for me, I am not a huge fan, and the practical effect is to
subtract from the amount of time that could otherwise have been spent
reviewing your next patch (or someone else's). In this case, I think
the best thing for me to do right now is wait to commit anything
further until you have had a chance to go over this and come up with a
fix or set of fixes that you think are completely, 100% ready to go;
or else until you get to the point of wanting feedback before
proceeding further. In general, I would appreciate if this sort of
post-commit self-review could be done pre-commit.

I apologize for the fact that this email probably sounds grouchy.
Please try to read it in the most positive light possible (and don't
shoot me).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company