From 732203f8d95ac2d29da9459a0a22f6ca656a4cf5 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Tue, 30 Sep 2025 00:29:13 +0200 Subject: [PATCH v20251001 17/25] simplify the compression header --- src/backend/storage/file/buffile.c | 223 ++++++++++++++++------------- 1 file changed, 121 insertions(+), 102 deletions(-) diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c index 125357fc07f..cb6763ed586 100644 --- a/src/backend/storage/file/buffile.c +++ b/src/backend/storage/file/buffile.c @@ -119,6 +119,23 @@ struct BufFile char *cBuffer; /* compression buffer */ }; +/* + * Header written right before each chunk of data with compression enabled. + * The 'len' is the length of the data buffer written right after the header, + * and 'raw_len' is the length of uncompressed data. If the data ends up not + * being compressed (e.g. when pglz does not reach the compression ratio), + * the raw_len is set to -1 and the len is the raw (uncompressed) length. + * + * To make things simpler, we write these headers even for mathods that do + * not fail (or rather when they fail, it's a proper error). The space for + * an extra integer seems negligible. + */ +typedef struct CompressHeader +{ + int len; /* data length (compressed, excluding header) */ + int raw_len; /* raw length (-1: not compressed) */ +} CompressHeader; + static BufFile *makeBufFileCommon(int nfiles); static BufFile *makeBufFile(File firstfile); static void extendBufFile(BufFile *file); @@ -271,11 +288,11 @@ BufFileCreateCompressTemp(bool interXact) { case TEMP_LZ4_COMPRESSION: #ifdef USE_LZ4 - size = LZ4_compressBound(BLCKSZ) + sizeof(int); + size = LZ4_compressBound(BLCKSZ) + sizeof(CompressHeader); #endif break; case TEMP_PGLZ_COMPRESSION: - size = pglz_maximum_compressed_size(BLCKSZ, BLCKSZ) + 2 * sizeof(int); + size = pglz_maximum_compressed_size(BLCKSZ, BLCKSZ) + sizeof(CompressHeader); break; } @@ -578,14 +595,14 @@ BufFileLoadBuffer(BufFile *file) { /* * Read and decompress data from a temporary file. We first read the - * length of compressed data, then the compressed data itself. + * header with compressed/raw lengths, and then the compressed data. */ int nread; - int nbytes; + CompressHeader header; nread = FileRead(thisfile, - &nbytes, - sizeof(nbytes), + &header, + sizeof(header), file->curOffset, WAIT_EVENT_BUFFILE_READ); @@ -594,7 +611,7 @@ BufFileLoadBuffer(BufFile *file) { /* eof, nothing to do */ } - else if (nread != sizeof(nbytes)) + else if (nread != sizeof(header)) { /* unexpected number of bytes, also covers (nread < 0) */ ereport(ERROR, @@ -604,97 +621,87 @@ BufFileLoadBuffer(BufFile *file) } else { - /* read length of compressed data, read and decompress data */ + /* read length of compressed data, read (and decompress) data */ char *buff = file->cBuffer; - int original_size = 0; Assert(file->cBuffer != NULL); /* advance past the length field */ - file->curOffset += sizeof(nbytes); + file->curOffset += sizeof(header); - /* For PGLZ, read additional original size */ - if (temp_file_compression == TEMP_PGLZ_COMPRESSION) + /* + * raw_len==-1 means the data was not compressed after all, which + * can happen e.g. for non-compressible data with pglz. In that + * case just copy the data in place. Otherwise do the decompression. + * + * XXX Maybe we should just do the FileRead first, and then either + * decompress or memcpy() for raw_len=-1. That'd be an extra memcpy, + * but it'd make the code simpler (this ways we do the error checks + * twice, for each branch). + */ + if (header.raw_len == -1) { - int nread_orig = FileRead(thisfile, - &original_size, - sizeof(original_size), - file->curOffset, - WAIT_EVENT_BUFFILE_READ); + nread = FileRead(thisfile, + file->buffer.data, + header.len, + file->curOffset, + WAIT_EVENT_BUFFILE_READ); + if (nread != header.len) + { + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not read file \"%s\": %m", + FilePathName(thisfile)))); + } + file->nbytes = nread; + file->curOffset += nread; + } + else + { /* - * Did we read the second (raw) length? We should not get an - * EOF here, we've already read the first length. + * Read compressed data into the separate buffer, and then + * decompress into the target file buffer. */ - if ((nread_orig == 0) || (nread_orig != sizeof(original_size))) + nread = FileRead(thisfile, + buff, + header.len, + file->curOffset, + WAIT_EVENT_BUFFILE_READ); + if (nread != header.len) { - /* also covers (nread_orig < 0) */ ereport(ERROR, (errcode_for_file_access(), errmsg("could not read file \"%s\": %m", FilePathName(thisfile)))); } - /* advance past the second length header */ - file->curOffset += sizeof(original_size); - - /* Check if data is uncompressed (marker = -1) */ - if (original_size == -1) + switch (temp_file_compression) { - /* FIXME this is missing error handling */ - int nread_data = FileRead(thisfile, - file->buffer.data, - nbytes, /* nbytes contains - * original size */ - file->curOffset, - WAIT_EVENT_BUFFILE_READ); - file->nbytes = nread_data; - file->curOffset += nread_data; + case TEMP_LZ4_COMPRESSION: +#ifdef USE_LZ4 + file->nbytes = LZ4_decompress_safe(buff, + file->buffer.data, header.len, + sizeof(file->buffer)); +#endif + break; - /* - * FIXME this is wrong, because it skips the track_io_timing - * stuff at the end, etc. - */ - return; + case TEMP_PGLZ_COMPRESSION: + file->nbytes = pglz_decompress(buff, header.len, + file->buffer.data, header.raw_len, false); + break; } - } - - /* - * Read compressed data, curOffset differs with pos It reads less - * data than it returns to caller So the curOffset must be - * advanced here based on compressed size - * - * XXX I don't understand what this comment is meant to say. Maybe - * it got broken by the earlier cleanup, not sure. - */ - nread = FileRead(thisfile, - buff, - nbytes, - file->curOffset, - WAIT_EVENT_BUFFILE_READ); + file->curOffset += nread; - switch (temp_file_compression) - { - case TEMP_LZ4_COMPRESSION: -#ifdef USE_LZ4 - file->nbytes = LZ4_decompress_safe(buff, - file->buffer.data, nbytes, sizeof(file->buffer)); -#endif - break; + if (file->nbytes < 0) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("compressed data is corrupt"))); - case TEMP_PGLZ_COMPRESSION: - file->nbytes = pglz_decompress(buff, nbytes, - file->buffer.data, original_size, false); - break; + /* should have got the expected length */ + Assert(file->nbytes == header.raw_len); } - file->curOffset += nread; - - if (file->nbytes < 0) - ereport(ERROR, - (errcode(ERRCODE_DATA_CORRUPTED), - errmsg_internal("compressed lz4 data is corrupt"))); } - } if (track_io_timing) @@ -734,15 +741,23 @@ BufFileDumpBuffer(BufFile *file) * buffer is enough. * * Then we simply point the "DataToWrite" buffer at the compressed buffer. + * + * XXX I'm not 100% happy with all the variables here, there seems to be + * more than necessary. */ if (file->compress) { char *cData; int cSize = 0; + CompressHeader header; Assert(file->cBuffer != NULL); cData = file->cBuffer; + /* initialize the header for compression */ + header.len = -1; + header.raw_len = nbytesOriginal; + switch (temp_file_compression) { case TEMP_LZ4_COMPRESSION: @@ -757,45 +772,49 @@ BufFileDumpBuffer(BufFile *file) * at the end. */ cSize = LZ4_compress_default(file->buffer.data, - cData + sizeof(int), file->nbytes, cBufferSize); + cData + sizeof(CompressHeader), + file->nbytes, cBufferSize); + if (cSize < 0) + { + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("compression failed, compressed size %d, original size %d", + cSize, nbytesOriginal))); + } #endif break; } case TEMP_PGLZ_COMPRESSION: cSize = pglz_compress(file->buffer.data, file->nbytes, - cData + 2 * sizeof(int), PGLZ_strategy_always); + cData + sizeof(CompressHeader), + PGLZ_strategy_always); + + /* + * pglz returns -1 for non-compressible data. In that case + * just copy the raw data into the output buffer. + */ + if (cSize == -1) + { + memcpy(cData + sizeof(CompressHeader), file->buffer.data, + header.raw_len); + + cSize = header.raw_len; + header.raw_len = -1; + } break; } - if (cSize <= 0) - { - ereport(ERROR, - (errcode(ERRCODE_DATA_CORRUPTED), - errmsg_internal("compression failed, compressed size %d, original size %d", - cSize, nbytesOriginal))); - } + Assert(cSize != -1); + header.len = cSize; /* - * Write the compressed length(s) at the beginning of the buffer. - * With lz4 we store just the compressed length, with pglz we store - * both the compressed and raw lengths (because pglz case fails if - * the compressed data would be larger, while lz4 always succeeds). - * - * XXX This seems like an unnecessary consistency. We could write - * both lengths in both cases, to unify the cases. It won't affect - * the efficiency too much, one more int seems negligible when - * compressing BLCKSZ worth of data. + * Write the header with compressed length at the beginning of the + * buffer. We store both the compressed and raw lengths, and use + * raw_len=-1 when the data was not compressed after all. */ - memcpy(cData, &cSize, sizeof(int)); - if (temp_file_compression == TEMP_PGLZ_COMPRESSION) - { - memcpy(cData + sizeof(int), &nbytesOriginal, sizeof(int)); - file->nbytes = cSize + 2 * sizeof(int); - } - else - { - file->nbytes = cSize + sizeof(int); - } + memcpy(cData, &header, sizeof(CompressHeader)); + file->nbytes = header.len + sizeof(CompressHeader); + DataToWrite = cData; } -- 2.51.0