On Thu, Oct 22, 2020 at 2:11 AM Tomas Vondra <tomas.von...@2ndquadrant.com> wrote: > > On Wed, Oct 21, 2020 at 01:59:50PM +0530, Dilip Kumar wrote: > >On Sat, Oct 17, 2020 at 11:34 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > >> > >> On Tue, Oct 13, 2020 at 10:30 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > >> > > >> > On Mon, Oct 12, 2020 at 7:32 PM Tomas Vondra > >> > <tomas.von...@2ndquadrant.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?
I don't think that we can throw an error here because pglz_compress might return -1 if it finds that it can not reduce the size of the data and we consider such data as "incompressible data" and return NULL. In such a case the caller will try to compress another attribute of the tuple. I think we can handle such cases in the specific handler functions. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
