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
signature.asc
Description: PGP signature
