Hi, On 3/17/23 03:43, Tomas Vondra wrote: > > ... > >>> I'm a little suspicious of the replacement of supports_compression() >>> with parse_compress_specification(). For example: >>> >>>> - errmsg = supports_compression(AH->compression_spec); >>>> - if (errmsg) >>>> + parse_compress_specification(AH->compression_spec.algorithm, >>>> + NULL, &compress_spec); >>>> + if (compress_spec.parse_error != NULL) >>>> { >>>> pg_log_warning("archive is compressed, but this installation does >>>> not support compression (%s >>>> - errmsg); >>>> - pg_free(errmsg); >>>> + compress_spec.parse_error); >>>> + pg_free(compress_spec.parse_error); >>>> } >>> >>> The top-level error here is "does not support compression", but wouldn't >>> a bad specification option with a supported compression method trip this >>> path too? >> >> No - since the 2nd argument is passed as NULL, it just checks whether >> the compression is supported. Maybe there ought to be a more >> direct/clean way to do it. But up to now evidently nobody needed to do >> that. >> > > I don't think the patch can use parse_compress_specification() instead > of replace supports_compression(). The parsing simply determines if the > build has the library, it doesn't say if a particular tool was modified > to support the algorithm. I might build --with-zstd and yet pg_dump does > not support that algorithm yet. > > Even after we add zstd to pg_dump, it's quite likely other compression > algorithms may not be supported by pg_dump from day 1. > > > I haven't looked at / tested the patch yet, but I wonder if you have any > thoughts regarding the size_t / int tweaks. I don't know what types zstd > library uses, how it reports errors etc. >
Any thoughts regarding my comments on removing supports_compression()? Also, this patch needs a rebase to adopt it to the API changes from last week. The sooner the better, considering we're getting fairly close to the end of the CF and code freeze. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company