Re: Different compression methods for FPI

On Fri, Mar 12, 2021 at 01:45:47AM -0600, Justin Pryzby wrote:
> On Sat, Mar 06, 2021 at 12:29:14PM +0500, Andrey Borodin wrote:
> > > 1 марта 2021 г., в 10:03, Justin Pryzby <pryzby(at)telsasoft(dot)com> написал(а):
> >
> > Justin, Michael, thanks for comments!
> >
> > As far as I understood TODO list for the patch looks as follows:
>
> Your patch can be simplified some, and then the only ifdef are in two short
> functions. Moving the compression calls to another function/file is hardly
> worth it, and anyone that implements a generic compression API could refactor
> easily, if it's a win. So I don't want to impose the burden on your small
> patch of setting up the compression API for everyone else's patches. Since
> this is non-streaming compression, the calls are trivial.
>
> One advantage of a generic API is that it's a good place to handle things like
> compression options, like zstd:9 or zstd:3,rsyncable (I am not suggesting this
> syntax).
>
> Today, I re-sent an Dillip's patch with a change to use pkg-config for liblz4,
> and it now also compiles on mac, so I used those changes to configure.ac (using
> pkg-config) and src/tools/msvc/Solution.pm, and changed HAVE_LIBLZ4 to USE_LZ4.

Updated patch with a minor fix to configure.ac to avoid warnings on OSX.
And 2ndary patches from another thread to allow passing recovery tests.
Renamed to WAL_COMPRESSION_*
Split LZ4 support to a separate patch and support zstd. These show that
changes needed for a new compression method have been minimized, although not
yet moved to a separate, abstracted compression/decompression function.

On Mon, Mar 01, 2021 at 01:57:12PM +0900, Michael Paquier wrote:
> > 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?

Michael: what frontend tools did you mean ?
pg_rewind? This may actually be okay as-is, since it uses symlinks:

$ ls -l src/bin/pg_rewind/xlogreader.c
lrwxrwxrwx 1 pryzbyj pryzbyj 48 Mar 12 17:48 src/bin/pg_rewind/xlogreader.c -> ../../../src/backend/access/transam/xlogreader.c

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

What do you mean? Are you concerned about alignment, or the extra width, or??

These two patches are a prerequisite for this patch to progress:
* Run 011_crash_recovery.pl with wal_level=minimal
* Make sure published XIDs are persistent

I don't know if anyone will consider this patch for v14 - if not, it should be
set to v15 and revisit in a month.

--
Justin