Re: Different compression methods for FPI

On Sun, Feb 28, 2021 at 05:08:17PM -0600, Justin Pryzby wrote:
> 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".

Good idea.  But are you sure that lz4 is available in the CF bot
environments?

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

Yeah, the patch feels incomplete with its footprint in xloginsert.c
for something that could be available in src/common/ like pglz,
particularly knowing that you will need to have this information 
available to frontend tools, no?

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

This declaration should be frontend-aware.  As presented, the patch
would enforce zlib or lz4 on top of pglz, but I guess that it would be
more user-friendly to complain when attempting to set up
wal_compression_method (why not just using wal_compression?) to an
unsupported option rather than doing it after-the-fact for the first
full page generated once the new parameter value is loaded.

This stuff could just add tests in src/test/recovery/.  See for
example src/test/ssl and with_ssl to see how conditional tests happen
depending on the configure options.

Not much a fan either of assuming that it is just fine to add one byte
to XLogRecordBlockImageHeader for the compression_method field.
--
Michael