Re: Add LZ4 compression in pg_dump - Mailing list pgsql-hackers
From | Justin Pryzby |
---|---|
Subject | Re: Add LZ4 compression in pg_dump |
Date | |
Msg-id | 20230225050214.GH1653@telsasoft.com Whole thread Raw |
In response to | Re: Add LZ4 compression in pg_dump (gkokolatos@pm.me) |
Responses |
Re: Add LZ4 compression in pg_dump
Re: Add LZ4 compression in pg_dump |
List | pgsql-hackers |
I have some fixes (attached) and questions while polishing the patch for zstd compression. The fixes are small and could be integrated with the patch for zstd, but could be applied independently. - I'm unclear about get_error_func(). That's called in three places from pg_backup_directory.c, after failures from write_func(), to supply an compression-specific error message to pg_fatal(). But it's not being used outside of directory format, nor for errors for other function pointers, or even for all errors in write_func(). Is there some reason why each compression method's write_func() shouldn't call pg_fatal() directly, with its compression-specific message ? - I still think supports_compression() should be renamed, or made into a static function in the necessary file. The main reason is that it's more clear what it indicates - whether compression is "implemented by pgdump" and not whether compression is "supported by this postgres build". It also seems possible that we'd want to add a function called something like supports_compression(), indicating whether the algorithm is supported by the current build. It'd be better if pgdump didn't subjugate that name. - Finally, the "Nothing to do in the default case" comment comes from Michael's commit 5e73a6048: + /* + * Custom and directory formats are compressed by default with gzip when + * available, not the others. + */ + if ((archiveFormat == archCustom || archiveFormat == archDirectory) && + !user_compression_defined) { #ifdef HAVE_LIBZ - if (archiveFormat == archCustom || archiveFormat == archDirectory) - compressLevel = Z_DEFAULT_COMPRESSION; - else + parse_compress_specification(PG_COMPRESSION_GZIP, NULL, + &compression_spec); +#else + /* Nothing to do in the default case */ #endif - compressLevel = 0; } As the comment says: for -Fc and -Fd, the compression is set to zlib, if enabled, and when not otherwise specified by the user. Before 5e73a6048, this set compressLevel=0 for -Fp and -Ft, *and* when zlib was unavailable. But I'm not sure why there's now an empty "#else". I also don't know what "the default case" refers to. Maybe the best thing here is to move the preprocessor #if, since it's no longer in the middle of a runtime conditional: #ifdef HAVE_LIBZ + if ((archiveFormat == archCustom || archiveFormat == archDirectory) && + !user_compression_defined) + parse_compress_specification(PG_COMPRESSION_GZIP, NULL, + &compression_spec); #endif ...but that elicits a warning about "variable set but not used"... -- Justin
Attachment
pgsql-hackers by date: