public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tomas Vondra <[email protected]>
To: [email protected]
Cc: Michael Paquier <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: [email protected]
Cc: Rachel Heaton <[email protected]>
Subject: Re: Add LZ4 compression in pg_dump
Date: Thu, 19 Jan 2023 18:55:43 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
References: <IZzs7Z2ny5hRK1EhTynSg3DnCo_z-UvEJN4Q6Hf0CUHWx3BcNwt-2z5vvvvA-Z8PQL-AYiL9YQPlKylATZ_Z6HnJ55YZUHgL66H9MZQFi48=@pm.me>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<twKgLFeqHEshCjdrbnzkwTMHiqfqvrrPSni7eKZjcxkForO-sI7HL4AKFgk9En1nGk3i4TXOY_An2xnCzv8MNjoLz4iupACaRB-0VRV20V4=@pm.me>
	<[email protected]>
	<ljb5G0qPPj_MM5y-C5fRT24uuuj6n5I5RY__MpT7JdQ_B_AIE8vIOgdRO1uiBee6cymDzFLAKbYh3wDFv86jprLhVKOdx70D4g_4JTjqnY0=@pm.me>
	<[email protected]>
	<7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>

Hi,

On 1/19/23 17:42, [email protected] wrote:
> 
> ------- Original Message -------
> On Thursday, January 19th, 2023 at 4:45 PM, Tomas Vondra <[email protected]> wrote:
>>
>> On 1/18/23 20:05, [email protected] wrote:
>>
>>> ------- Original Message -------
>>> On Wednesday, January 18th, 2023 at 3:00 PM, Tomas Vondra [email protected] 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

view thread (40+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add LZ4 compression in pg_dump
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox