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

On Mon, Apr 7, 2014 at 4:35 PM, Peter Geoghegan <pg@heroku.com> wrote:
> The much earlier datum1 optimization is mostly compelling for
> pass-by-value types, for reasons that prominently involve
> cache/locality considerations.

I agree.

> That's probably why this patch is so
> compelling - it makes those advantages apply to pass-by-reference
> types too.

Well, whether the patch is compelling is actually precisely what we
need to figure out.  I feel like you're asserting your hoped-for
conclusion prematurely.

>> Testing the cases where your patch wins and hand-waving that the
>> losses won't be that bad in other cases - without actually testing -
>> is not the right methodology.
>
> Okay. Here is a worst-case, with the pgbench script the same as my
> original test-case, but with much almost maximally unsympathetic data
> to sort:
>
> [local]/postgres=# update customers set firstname =
> 'padding-padding-padding-padding' || firstname;
> UPDATE 20000
>
> Master:
>
> pg@hamster:~/sort-tests$ pgbench -f sort.sql -n -T 100
> transaction type: Custom query
> scaling factor: 1
> query mode: simple
> number of clients: 1
> number of threads: 1
> duration: 100 s
> number of transactions actually processed: 323
> latency average: 309.598 ms
> tps = 3.227745 (including connections establishing)
> tps = 3.227784 (excluding connections establishing)
>
> Patch:
>
> pg@hamster:~/sort-tests$ pgbench -f sort.sql -n -T 100
> transaction type: Custom query
> scaling factor: 1
> query mode: simple
> number of clients: 1
> number of threads: 1
> duration: 100 s
> number of transactions actually processed: 307
> latency average: 325.733 ms
> tps = 3.066256 (including connections establishing)
> tps = 3.066313 (excluding connections establishing)
>
> That seems like a pretty modest regression for a case where 100% of
> what I've done goes to waste. If something that I'd done worked out
> 10% of the time, we'd still be well ahead.

Now that is definitely interesting, and it does seem to demonstrate
that the worst case for this patch might not be as bad as I had feared
- it's about a 5% regression: not great, but perhaps tolerable.  It's
not actually a worse case unless firstname is a fair ways into a tuple
with lots of variable-length columns before it, because part of what
the datum1 thing saves you is the cost of repeatedly walking through
the tuple's column list.

But I still think that a lot more could be done - and I'd challenge
you (or others) to do it - to look for cases where this might be a
pessimization.  I get that the patch has an upside, but nearly every
patch that anybody proposes does, and the author usually points out
those cases quite prominently, as you have, and right so.  But what
makes for a much more compelling submission is when the author *also*
tries really hard to break the patch, and hopefully fails.  I agree
that the test result shown above is good news for this patch's future
prospects, but I *don't* agree that it nails the door shut.  What
about other locales?  Other operating systems?  Other versions of
libc?  Longer strings?  Wider tuples?  Systems where datums are only
32-bits?  Sure, you can leave those things to the reviewer and/or
committer to worry about, but that's not the way to expedite the
patch's trip into the tree.

I have to admit, I didn't really view the original postings on this
topic to be anything more than, hey, we've got something promising
here, it's worth more study.  That's part of why I was so taken aback
by Greg's email.  There's certainly good potential here, but I think
there's quite a bit left of work left to do before you can declare
victory...

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