Horton

Re: [HACKERS] Custom compression methods

Dilip Kumar <dilipbalaut@gmail.com>

View thread View on postgr.es
From: Dilip Kumar <dilipbalaut@gmail.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, David Steele <david@pgmasters.net>, Ildus Kurbangaliev <i.kurbangaliev@gmail.com>, Dmitry Dolgov <9erthalion6@gmail.com>, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Date: 2020-10-22T12:26:55Z
Lists: pgsql-hackers

Attachments

On Thu, Oct 22, 2020 at 10:41 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Oct 22, 2020 at 2:11 AM Tomas Vondra
> <tomas.vondra@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 <dilipbalaut@gmail.com> wrote:
> > >>
> > >> On Tue, Oct 13, 2020 at 10:30 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >> >
> > >> > On Mon, Oct 12, 2020 at 7:32 PM Tomas Vondra
> > >> > <tomas.vondra@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.

I have added the compression failure error in lz4.c, please refer
lz4_cmcompress in v9-0001 patch.  Apart from that, I have also
supported the PRESERVE ALL syntax to preserve all the existing
compression methods.  I have also rebased the patch on the current
head.

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

Commits

  1. docs: Update TOAST storage docs for configurable compression.

  2. Further tweaking of pg_dump's handling of default_toast_compression.

  3. Fix interaction of TOAST compression with expression indexes.

  4. Tidy up more loose ends related to configurable TOAST compression.

  5. Short-circuit slice requests that are for more than the object's size.

  6. Mostly-cosmetic adjustments of TOAST-related macros.

  7. Remove useless configure probe for <lz4/lz4.h>.

  8. Error on invalid TOAST compression in CREATE or ALTER TABLE.

  9. docs: Fix omissions related to configurable TOAST compression.

  10. More code cleanup for configurable TOAST compression.

  11. Bring configure support for LZ4 up to snuff.

  12. Make compression.sql regression test independent of default.

  13. Use valid compression method in brin_form_tuple

  14. Fix up pg_dump's handling of per-attribute compression options.

  15. Allow configurable LZ4 TOAST compression.

  16. Fix inconsistencies in the code

  17. Mostly-cosmetic improvements in memory chunk header alignment coding.

  18. Allow numeric to use a more compact, 2-byte header in many cases.