Re: [HACKERS] Custom compression methods

On Wed, Oct 21, 2020 at 01:59:50PM +0530, Dilip Kumar wrote:
>On Sat, Oct 17, 2020 at 11:34 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>>
>> On Tue, Oct 13, 2020 at 10:30 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>> >
>> > On Mon, Oct 12, 2020 at 7:32 PM Tomas Vondra
>> > <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>> > >
>> > > On Mon, Oct 12, 2020 at 02:28:43PM +0530, Dilip Kumar wrote:
>> > > >
>> > > >> ...
>> > > >
>> > > >I have worked on this patch, so as discussed now I am maintaining the
>> > > >preserved compression methods using dependency. Still PRESERVE ALL
>> > > >syntax is not supported, I will work on that part.
>> > > >
>> > >
>> > > Cool, I'll take a look. What's your opinion on doing it this way? Do you
>> > > think it's cleaner / more elegant, or is it something contrary to what
>> > > the dependencies are meant to do?
>> >
>> > I think this looks much cleaner. Moreover, I feel that once we start
>> > supporting the custom compression methods then we anyway have to
>> > maintain the dependency so using that for finding the preserved
>> > compression method is good option.
>>
>> I have also implemented the next set of patches.
>> 0004 -> Provide a way to create custom compression methods
>> 0005 -> Extention to implement lz4 as a custom compression method.
>
>In the updated version I have worked on some of the listed items
>> A pending list of items:
>> 1. Provide support for handling the compression option
>> - As discussed up thread I will store the compression option of the
>> latest compression method in a new field in pg_atrribute table
>> 2. As of now I have kept zlib as the second built-in option and lz4 as
>> a custom compression extension. In Offlist discussion with Robert, he
>> suggested that we should keep lz4 as the built-in method and we can
>> move zlib as an extension because lz4 is faster than zlib so better to
>> keep that as the built-in method. So in the next version, I will
>> change that. Any different opinion on this?
>
>Done
>
>> 3. Improve the documentation, especially for create_compression_method.
>> 4. By default support table compression method for the index.
>
>Done
>
>> 5. Support the PRESERVE ALL option so that we can preserve all
>> existing lists of compression methods without providing the whole
>> list.
>
>1,3,5 points are still pending.
>

Thanks. I took a quick look at the patches and I think it seems fine. I
have one question, though - toast_compress_datum contains this code:

/* Call the actual compression function */
tmp = cmroutine->cmcompress((const struct varlena *) value);
if (!tmp)
return PointerGetDatum(NULL);

Shouldn't this really throw an error instead? I mean, if the compression
library returns NULL, isn't that an error?

regards

>--
>Regards,
>Dilip Kumar
>EnterpriseDB: http://www.enterprisedb.com