Re: ZSTD TOAST compression, and an extensible compression method encoding

Hi Manu,

Thanks for the integrated 0003 and the evidence file. I went through
the change and re-ran the parts that matter on a few differently shaped
hosts I have at home with v7 and your 0003 side by side on 54635e6892f.

Re: reset, I took your point that it is what makes the reuse safe and
checked it without any corruption: in one backend, take a 10-byte
prefix of one 600kB value, then a 300kB slice of another. v7 and your
0003 both return the right bytes. With the ZSTD_DCtx_reset() call
removed, every slice after the prefix fails with "compressed zstd data
is corrupt", while full reads stay correct. I didn't reproduce the
silent wrong-data case you saw, only the false error, but either is
reason enough. An error on a corrupted value, inside a savepoint or
not, left later reads correct with the reset in place.

Speed, -O2, six rounds alternating builds, time with reuse over v7:

32kB values, 100-byte prefix 0.62
32kB values, slice at 10kB 0.62
32kB values, full read 0.96
4kB values, 100-byte prefix 0.66
1MB values, slice at 500kB 0.99

So it matches your numbers for slices and prefixes. Full reads gain
less here than in yours, which I'd put down to my data compressing less
well. ZSTD_sizeof_DCtx() gives 96kB for a fresh context, unchanged
after one-shot reads, and 1.2MB after a 300kB slice of a 1MB value,
also in line with what you measured.

I folded it into 0003 for v8, attached, with you as Co-authored-by.
Besides your change, 0003 now differs from v7 only in two places:

- The comment above zstd_dctx says the reset is required rather than
an optimization, and that anything else using the context has to go
through zstd_get_dctx(). It explains why the scope is the backend
(detoasting has no owner like the XLogReaderState that ce3387b74b7
uses), and it notes that zstd allocates the context with malloc(), so
its memory doesn't show in pg_backend_memory_contexts. I expect a
reviewer to ask about the last point first.

- The commit message says the same things briefly, and gives the
speedup.

In 0004, the comment that suggested moving dictionary prefixes to the
streaming loop now says that dict_dctx would then need the same reset
before each use, and why. As you pointed out, the one-shot call it
uses today doesn't need one.

v8 is on master as of 45fc51fcbf4:

0001 Decode the compression method in toast_external_info_get()
0002 Allow more than four TOAST compression methods
0003 Add zstd as a TOAST compression method (now with context reuse)
0004 Add zstd compression dictionaries with a column-local lifecycle

0001 and 0002 are v7 unchanged. Each patch builds on its own with
-Werror, cassert and injection points, and passes the regression tests.
check-world on 0004 passes (387 tests, none failed), and so does a build
without zstd.

Nikhil, the first three are still yours; I only rebased them and added
Manu's change to 0003. I hope to see you rejoin the conversation and
lead the development again.

best.

-greg