Re: Add LZ4 compression in pg_dump

Hi,

On 1/19/23 17:42, gkokolatos(at)pm(dot)me wrote:
>
> ------- Original Message -------
> On Thursday, January 19th, 2023 at 4:45 PM, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>
>> On 1/18/23 20:05, gkokolatos(at)pm(dot)me wrote:
>>
>>> ------- Original Message -------
>>> On Wednesday, January 18th, 2023 at 3:00 PM, Tomas Vondra tomas(dot)vondra(at)enterprisedb(dot)com wrote:
>>
>> I'm not sure I understand why leave the lz4/zstd in this place?
>
> You are right, it is not obvious. Those were added in 5e73a60488 which is
> already committed in master and I didn't want to backtrack. Of course, I am
> not opposing in doing so if you wish.
>

Ah, I didn't realize it was already added by earlier commit. In that
case let's not worry about it.

>>
>>>> 2) I wouldn't reorder the cases in WriteDataToArchive, i.e. I'd keep
>>>> "none" at the end. It might make backpatches harder.
>>>
>>> Agreed. However a 'default' is needed in order to avoid compilation warnings.
>>> Also note that 0002 completely does away with cases within WriteDataToArchive.
>>
>>
>> OK, although that's also a consequence of using a "switch" instead of
>> plan "if" branches.
>>
>> Furthermore, I'm not sure we really need the pg_fatal() about invalid
>> compression method in these default blocks. I mean, how could we even
>> get to these places when the build does not support the algorithm? All
>> of this (ReadDataFromArchive, WriteDataToArchive, EndCompressor, ...)
>> happens looong after the compressor was initialized and the method
>> checked, no? So maybe either this should simply do Assert(false) or use
>> a different error message.
>
> I like Assert(false).
>

OK, good. Do you agree we should never actually get there, if the
earlier checks work correctly?

>>
>>>> 4) "cfp" struct no longer wraps gzFile, but the comment was not updated.
>>>> FWIW I'm not sure switching to "void *" is an improvement, maybe it'd be
>>>> better to have a "union" of correct types?
>>>
>>> Please find and updated comment and a union in place of the void *. Also
>>> note that 0002 completely does away with cfp in favour of a new struct
>>> CompressFileHandle. I maintained the void * there because it is used by
>>> private methods of the compressors. 0003 contains such an example with
>>> LZ4CompressorState.
>>
>>
>> I wonder if this (and also the previous item) makes sense to keep 0001
>> and 0002 or to combine them. The "intermediate" state is a bit annoying.
>
> Agreed. It was initially submitted as one patch. Then it was requested to be
> split up in two parts, one to expand the use of the existing API and one to
> replace with the new interface. Unfortunately the expansion of usage of the
> existing API requires some tweaking, but that is not a very good reason for
> the current patch set. I should have done a better job there.
>
> Please find v22 attach which combines back 0001 and 0002. It is missing the
> documentation that was discussed above as I wanted to give a quick feedback.
> Let me know if you think that the combined version is the one to move forward
> with.
>

Thanks, I'll take a look.

regards

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