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