Date: Sun, 28 Feb 2021 17:08:17 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Different compression methods for FPI
Message-ID: <20210228230817.GE20769@telsasoft.com>
References: <3037310D-ECB7-4BF1-AF20-01C10BB33A33@yandex-team.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3037310D-ECB7-4BF1-AF20-01C10BB33A33@yandex-team.ru>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Sat, Feb 27, 2021 at 12:43:52PM +0500, Andrey Borodin wrote:
> So I think it worth to propose a patch to make wal_compression_method = {"pglz", "lz4", "zlib"}. Probably, "zstd" can be added to the list.
> Attached is a draft taking CompressionId from "custom compression methods" patch and adding zlib to it.

Thanks for submitting it.

Does this need to patch ./configure{,.ac} and Solution.pm for HAVE_LIBLZ4 ?
I suggest to also include an 0002 patch (not for commit) which changes to use a
non-default compression, to exercise this on the CIs - linux and bsd
environments now have liblz4 installed, and for windows you can keep "undef".

Daniil had a patch to add src/common/z_stream.c:
https://github.com/postgrespro/libpq_compression/blob/0a9c70d582cd4b1ef60ff39f8d535f6e800bd7d4/src/common/z_stream.c
https://www.postgresql.org/message-id/470E411E-681D-46A2-A1E9-6DE11B5F59F3@yandex-team.ru

Your patch looks fine, but I wonder if we should first implement a generic
compression API.  Then, the callers don't need to have a bunch of #ifdef.
If someone calls zs_create() for an algorithm which isn't enabled at compile
time, it throws the error at a lower level.

That also allows a central place to do things like handle options (compression
level, and things like zstd --long, --rsyncable, etc).

In some cases there's an argument that the compression ID should be globally
defined constant, not like a dynamic "plugable" OID.  That may be true for the
libpq compression, WAL compression, and pg_dump, since there's separate
"producer" and "consumer".  I think if there's "pluggable" compression (like
the TOAST patch), then it may have to map between the static or dynamic OIDs
and the global compression ID.

-- 
Justin