On Tue, Nov 26, 2019 at 08:17:13PM +0100, Peter Eisentraut wrote:
On 2019-11-26 10:43, Tomas Vondra wrote:
In general, I think the results for both patches seem clearly a win, but
maybe patch 1 is bit better, especially on the newer (xeon) CPU. So I'd
probably go with that one.
Patch 1 is also the simpler patch, so it seems clearly preferable.
Yeah, although the difference is minimal. We could probably construct a
benchmark where #2 wins, but I think these queries are fairly realistic.
So I'd just go with #1.
Code-wise I think the patches are mostly fine, although the comments
might need some proof-reading.
1) I wasn't really sure what a "nibble" is, but maybe it's just me and
it's a well-known term.
2) First byte use lower -> First byte uses lower
3) nibble contain upper -> nibble contains upper
4) to preven possible uncertanity -> to prevent possible uncertainty
5) I think we should briefly explain why memmove would be incompatible
with pglz, it's not quite clear to me.
6) I'm pretty sure the comment in the 'while (off < len)' branch will be
badly mangled by pgindent.
7) The last change moving "copy" to the next line seems unnecessary.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
