Re: Add LZ4 compression in pg_dump

On 2/25/23 06:02, Justin Pryzby wrote:
> 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 think there are a couple more places that might/should call
get_error_func(). For example ahwrite() in pg_backup_archiver.c now
simply does

if (bytes_written != size * nmemb)
WRITE_ERROR_EXIT;

but perhaps it should call get_error_func() too. There are probably
other places that call write_func() and should use get_error_func().

> - 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.
>

If we choose to rename this to have pgdump_ prefix, fine with me. But I
don't think there's a realistic chance of conflict, as it's restricted
to pgdump header etc. And it's not part of an API, so I guess we could
rename that in the future if needed.

> - 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"...
>

Not sure, I need to think about this a bit.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company