On Wed, Jun 5, 2013 at 8:01 AM, Andres Freund <and...@2ndquadrant.com>wrote:
>
> Two patches attached:
> 1) add snappy to src/common. The integration needs some more work.
> 2) Combined patch that adds indirect tuple and snappy compression. Those
> coul be separated, but this is an experiment so far...
>
>
>
I took a look at them a little. This proposal is a super set of patch
#1127.
https://commitfest.postgresql.org/action/patch_view?id=1127
- <endian.h> is not found in my mac. Commented it out, it builds clean.
- I don't see what the added is_inline flag means in
toast_compress_datum(). Obviously not used, but I wonder what was the
intention.
- By this,
* compression method. We could just use the two bytes to store 3 other
* compression methods but maybe we better don't paint ourselves in a
* corner again.
you mean two bits, not two bytes?
And patch adds snappy-c in src/common. I definitely like the idea to have
pluggability for different compression algorithm for datum, but I am not
sure if this location is a good place to add it. Maybe we want a modern
algorithm other than pglz for different components across the system in
core, and it's better to let users choose which to add more. The mapping
between the index number and compression algorithm should be consistent for
the entire life of database, so it should be defined at initdb time. From
core maintainability perspective and binary size of postgres, I don't think
we want to put dozenes of different algorithms into core in the future.
And maybe someone will want to try BSD-incompatible algorithm privately.
I guess it's ok to use one more byte to indicate which compression is used
for the value. It is a compressed datum and we don't expect something
short anyway. I don't see big problems in this patch other than how to
manage the pluggability, but it is a WIP patch anyway, so I'm going to mark
it as Returned with Feedback.
Thanks,
--
Hitoshi Harada
