Re: [HACKERS] Custom compression methods

On Tue, Mar 9, 2021 at 1:56 AM Robert Haas <robertmhaas(at)gmail(dot)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