Re: Proposal: Adding compression of temporary files

Hello Filip,

Thanks for the updated patch, and for your patience with working on this
patch with (unfortunately) little feedback. I took a look at the patch,
and did some testing. In general, I think it's heading in the right
direction, but there's still a  couple issues and open questions.

Attached is a collection of incremental patches with the proposed
changes. I'll briefly explain the motivation for each patch, but it's
easier to share the complete change as a patch. Feel free to disagree
with the changes, some are a matter of opinion, and/or there might be a
better way to do that. Ultimately it should be squashed to the main
patch, or perhaps a couple larger patches.

v20250930-0001-Add-transparent-compression-for-temporary-.patch

- original patch, as posted on 2025/09/26


v20250930-0002-whitespace.patch

- cleanup of whitespace issues
- This is visible in git-show or when applying using git-am.


v20250930-0003-pgindent.patch

- pgindent run, to fix code style (formatting of if/else branches,
indentation, that kind of stuff)
- good to run pgindent every now and then, for consistency


v20250930-0004-Add-regression-tests-for-temporary-file-co.patch

- original patch, as posted on 2025/09/26

v20250930-0005-remove-unused-BufFile-compress_tempfile.patch

- the compress_tempfile was unused, get rid of it


v20250930-0006-simplify-BufFileCreateTemp-interface.patch

- I think the proposed interface (a "compress" flag in BufFileCreateTemp
and then a separate method BufFileCreateCompressTemp) is not great.

- The "compress" flag is a bit pointless, because even if you set it to
"true" you won't get compressed file. In fact, it's fragile - you'll get
broken BufFile without the buffer.

- The patch gets rid of the "compress" flag (so existing callers of
BufFileCreateTemp remain unchanged). BufFileCreateCompressTemp sets the
flag directly, which it can because it's in the same module.

- An alternative would be to keep the flag, do all the compression setup
in BufFileCreateTemp, and get rid of BufFileCreateCompressTemp. Not sure
which is better.


v20250930-0007-improve-BufFileCreateTemp-BufFileCreateCom.patch

- Just improving comments, to document the new stuff (needs a check).

- There are two new XXX comments, with questions. One asks if the
allocation is an issue in practice - is the static buffer worth it? The
other suggests we add an assert protecting against unsupported seeks.


v20250930-0008-BufFileCreateCompressTemp-cleanup-and-comm.patch

- A small BufFileCreateCompressTemp cleanup (better comments, better
variable names, formatting, extra assert, ... mostly cosmetic stuff).

- But this made me realize the 'static buffer' idea is likely flawed, at
least the current code. It does pfree() on the current buffer, but how
does it know if there are other files referencing it? Because it then
stashes the buffer to file->buffer. I haven't tried to reproduce the
issue, nor fixed this, but it seems like it might be a problem if two
files happen to use a different compression method.


v20250930-0009-minor-BufFileLoadBuffer-cleanup.patch

- Small BufFileLoadBuffer cleanup, I don't think it's worth it to have
such detailed error messages. So just use "could not read file" and then
whatever libc appends as %m.


v20250930-0010-BufFileLoadBuffer-simpler-FileRead-handlin.patch
v20250930-0011-BufFileLoadBuffer-simpler-FileRead-handlin.patch

- I was somewhat confused by the FileRead handling in BufFileLoadBuffer,
so these two patches try to improve / clarify it.

- I still don't understand the purpose of the "if (nread_orig <= 0)"
branch removed by the second patch.


v20250930-0012-BufFileLoadBuffer-comment-update.patch

- minor comment tweak


v20250930-0013-BufFileLoadBuffer-simplify-skipping-header.patch

- I found it confusing how the code advanced the offset by first adding
to header_advance, and only then adding to curOffset much later. This
gets rid of that, and just advances curOffset right after each read.


v20250930-0014-BufFileDumpBuffer-cleanup-simplification.patch

- Improve the comments in BufFileDumpBuffer, and simplify the code. This
is somewhat subjective, but I think the code is more readable.

- It temporarily removes the handling of -1 for pglz compression. This
was a mistake, and is fixed by a later patch.


v20250930-0015-BufFileLoadBuffer-comment.patch

- XXX for a comment I don't understand.


v20250930-0016-BufFileLoadBuffer-missing-FileRead-error-h.patch

- Points out a FileRead call missing error handling (there's another one
with the same issue).


v20250930-0017-simplify-the-compression-header.patch

- I came to the conclusion that having one "length" field for lz4 and
two (compressed + raw) for pglz makes the code unnecessarily complex,
without gaining much. So this just adds a "header" struct with both
lengths for all compression algorithms. I think it's cleaner/simpler.


v20250930-0018-undo-unncessary-changes-to-Makefile.patch

- Why did the 0001 patch add this? Maybe it's something we should add
separately, not as part of this patch?


v20250930-0019-enable-compression-for-tuplestore.patch

- Enables compression for tuplestores that don't require random access.
This covers e.g. tuplestores produces by SRF like generate_series, etc.

- I still wonder what would it take to support random access. What if we
remember offsets of each block? We could write that into an uncompressed
file. That'd be 128kB per 1GB, which seems acceptable. Just a thought.


v20250930-0020-remember-compression-method-for-each-file.patch

- The code only tracked bool "compress" flag for each file, and then
determined the algorithm during compression/decompression based on the
GUC variable. But that's incorrect, because the GUC can change during
the file life time. For example, there can be a cursor, anb you can do
SET temp_file_compression between the FETCH calls (see the commit
message for an example).

- So this replaces the flag with storing the actual method.


v20250930-0021-LZ4_compress_default-returns-0-on-error.patch

- The LZ4_compress_default returns 0 in case of error. Probably a bug
introduced by one of my earlier patches.


v20250930-0022-try-LZ4_compress_fast.patch

- Experimental patch, trying a faster LZ4 compression.


So that's what I have at the moment. I'm also doing some testing,
measuring the effect of compression both for trivial queries (based on
generate_series) and more complex ones from TPC-H.

I'll post the complete results when I have that, but the results I've
seen so far show that:

- pglz and lz4 end up with about the same compression ratio (in TPC-H
it's often cutting the temp files in about half)

- lz4 is on par with no compression (it's pretty much within noise),
while pglz is much slower (sometimes ~2x slower)


I wonder how would gzip/zstandard perform. My guess would be that gzip
would be faster than pglz, but still slower than lz4. Zstandard is much
closer to lz4. Would it be possible to have some experimental patches
for gzip/zstd, so that we can try? It'd also validate that the code is
prepared for adding more algorithms in the future.

The other thing I was thinking about was the LZ4 stream compression.
There's a comment suggesting it might compress better, and indeed - when
working on pg_dump compression we saw a huge improvement. Again, would
be great to support have an experimental patch for this, so that we can
evaluate it.



regards

-- 
Tomas Vondra