Horton

Re: [HACKERS] Custom compression methods

Dilip Kumar <dilipbalaut@gmail.com>

View thread View on postgr.es
From: Dilip Kumar <dilipbalaut@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, David Steele <david@pgmasters.net>, Ildus Kurbangaliev <i.kurbangaliev@gmail.com>, Dmitry Dolgov <9erthalion6@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-03-10T11:51:45Z
Lists: pgsql-hackers

Attachments

On Tue, Mar 9, 2021 at 1:56 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> Some other review comments:

I have worked on these review comments. Please find my response inline
>
> toast_get_compression_method() should now return char, not Oid.

Fixed

> With this design, we can support changing the compression method on a
> column quite easily. It's just a hint, like the STORAGE parameter. It
> has no bearing on what can be present in the table, but just controls
> how new values are stored. It would be nice to have a way to force
> anything compressed with the old method to be re-compressed with the
> new method, but not having that doesn't preclude allowing the
> parameter to be changed.

As responded upthread, as of now I am planning to provide a syntax as

ALTER COLUMN col SET COMPRESSION method REWRITE, if user wants to
rewrite the table.

> I am tempted to propose that we collapse compress_lz4.c and
> compress_pglz.c into a single file, get rid of the directory, and just
> have something like src/backend/access/common/toast_compression.c. The
> files are awfully short, and making a whole new directory for that
> small amount of code seems like overkill.

I have done that, along with that I have also renamed compressapi.h to
toast_compression.h, and along with that I have done some more
refactoring of the code especially in toast_compression.c and
toast_compression.h,  please have a look.

> I think the pg_dump argument should be --no-toast-compression, not
> --no-toast-compressions.

Done

I agree with Justin that pg_restore should
> have the option also.

Not done anything for pg_restore as we already agreed upon this.

> Man, it would be really nice to be able to set the default for new
> tables, rather than having all these places hard-coded to use
> DefaultCompressionMethod. Surely lotsa people are going to want to set
> toast_compression = lz4 in postgresql.conf and forget about it.

As Justine pointed out we are doing in 0002, maybe we should merge
0001 and 0002 but I kept is that way so that the review can be easy.

> Is there any reason not to change varattrib_4b's description of
> va_tcinfo that says "and flags" to instead say "and compression
> method"? And rename VARFLAGS_4B_C to VARCOMPRESS_4B_C? I don't know
> why we should call it flags when we know it's specifically compression
> information.

Done.

> You should probably have a test that involves altering the type of a
> varlena column to non-varlena, and the other way around, and make sure
> that changing integer -> text sets attcompression and doing the
> reverse clears it.

Done, also added the test case to see that setting the storage type to
plain on varlena type should not clear the compression method.

> You need to update catalogs.sgml.

Done

> On the whole I don't see a whole lot to complain about here. I don't
> love giving up on the idea of tracking which compression methods are
> used where, but making that work without performance regressions seems
> very difficult and perhaps just outright impossible, and dealing with
> all the concurrency problems that introduces is a pain, too. I think
> accepting a feature that gives us LZ4 compression is better than
> rejecting it because we can't solve those problems.

Right.

Apart from this I have also fixed the comment given by Justin.

The pending comment is providing a way to rewrite a table and
re-compress the data with the current compression method.

-- 
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.