Message-ID: <d8cbc586-a8e3-1662-c158-9022d5c8de83@enterprisedb.com>
Date: Thu, 19 Jan 2023 18:55:43 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: Add LZ4 compression in pg_dump
Content-Language: en-US
To: gkokolatos@pm.me
Cc: Michael Paquier <michael@paquier.xyz>,
 Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org,
 Rachel Heaton <rachelmheaton@gmail.com>
References: <IZzs7Z2ny5hRK1EhTynSg3DnCo_z-UvEJN4Q6Hf0CUHWx3BcNwt-2z5vvvvA-Z8PQL-AYiL9YQPlKylATZ_Z6HnJ55YZUHgL66H9MZQFi48=@pm.me>
 <20230114214308.GC9837@telsasoft.com> <Y8SoUuqbNoMBnFJp@paquier.xyz>
 <20230116015625.GH9837@telsasoft.com> <Y8S8dkXvymKauvUh@paquier.xyz>
 <twKgLFeqHEshCjdrbnzkwTMHiqfqvrrPSni7eKZjcxkForO-sI7HL4AKFgk9En1nGk3i4TXOY_An2xnCzv8MNjoLz4iupACaRB-0VRV20V4=@pm.me>
 <41036470-041b-08a3-469d-fe07de2bc58e@enterprisedb.com>
 <ljb5G0qPPj_MM5y-C5fRT24uuuj6n5I5RY__MpT7JdQ_B_AIE8vIOgdRO1uiBee6cymDzFLAKbYh3wDFv86jprLhVKOdx70D4g_4JTjqnY0=@pm.me>
 <6ab89d42-0345-b96e-02d4-68e171cdbe03@enterprisedb.com>
 <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/d8cbc586-a8e3-1662-c158-9022d5c8de83%40enterprisedb.com>
Precedence: bulk

Hi,

On 1/19/23 17:42, gkokolatos@pm.me wrote:
> 
> ------- Original Message -------
> On Thursday, January 19th, 2023 at 4:45 PM, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
>>
>> On 1/18/23 20:05, gkokolatos@pm.me wrote:
>>
>>> ------- Original Message -------
>>> On Wednesday, January 18th, 2023 at 3:00 PM, Tomas Vondra tomas.vondra@enterprisedb.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