Message-ID: <3beb0dd7-c927-3eca-689f-716e255d5b27@enterprisedb.com>
Date: Thu, 23 Mar 2023 18:10:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: Add LZ4 compression in pg_dump
To: Justin Pryzby <pryzby@telsasoft.com>, gkokolatos@pm.me
Cc: Alexander Lakhin <exclusion@gmail.com>, shiy.fnst@fujitsu.com,
 Michael Paquier <michael@paquier.xyz>, pgsql-hackers@lists.postgresql.org,
 Rachel Heaton <rachelmheaton@gmail.com>
References: 
 <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
 <8421f53d-c9f7-c02e-3278-9f9522a8e8f3@enterprisedb.com>
 <33496f7c-3449-1426-d568-63f6bca2ac1f@gmail.com>
 <1aBKUlb8BGVUUTkRJwEmU3ud1NyeHVCoC7bpOKAuRYlHHJ7hRiDVXYgad-wmjx1K3pykd_ctyY4Lqy9xA7JGLXOm1Eul-imdZ_7TVr5LY8U=@pm.me>
 <3da84c90-7e12-75ee-a582-025bc68dd098@enterprisedb.com>
 <5qAu7vrMV3hEodlIC82b0JICWTwEUSRoIDWlIaczNBIEVVfDH2lccxHNIQ726e4nX_Qqjc4lPc5_z9RtV28NMdVv7BNiADF4JB-FDahmlVk=@pm.me>
 <d8315656-3b35-7d9a-c799-8d48fdccfe45@enterprisedb.com>
 <JduQzRIUMhF5NDvC_klfyHTDhzb5Py60xH9ICRUtluHQ4dD02nw-BH7WfX7DQG6gaFWl1Q919RvX3Xvm_5mau_0YKRQeUs6ezUSFDlFVPsE=@pm.me>
 <32ba9612-1982-4b2d-135a-5d1222987082@enterprisedb.com>
 <XPMIIorJ5munrlLRZGquartiN5tuapdjiBhaPZ6iEmJccRiKWCIHSowSLPhaGj4gLpqvaS1pQn7PGkA_WNwvuUTVdl-lrU_TzHA81LqujJ4=@pm.me>
 <ZBjg4D/X05X+JbcJ@telsasoft.com>
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <ZBjg4D/X05X+JbcJ@telsasoft.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/3beb0dd7-c927-3eca-689f-716e255d5b27%40enterprisedb.com>
Precedence: bulk

Hi,

I looked at this again, and I realized I misunderstood the bit about
errno in LZ4File_open_write a bit. I now see it simply just brings the
function in line with Gzip_open_write(), so that the callers can just do
pg_fatal("%m"). I still think the special "errno" handling in this one
place feels a bit random, and handling it by get_error_func() would be
nicer, but we can leave that for a separate patch - no need to block
these changes because of that.

So pushed all three parts, after updating the commit messages a bit.

This leaves the empty-data issue (which we have a fix for) and the
switch to LZ4F. And then the zstd part.


On 3/20/23 23:40, Justin Pryzby wrote:
> On Fri, Mar 17, 2023 at 03:43:58PM +0000, gkokolatos@pm.me wrote:
>> From a174cdff4ec8aad59f5bcc7e8d52218a14fe56fc Mon Sep 17 00:00:00 2001
>> From: Georgios Kokolatos <gkokolatos@pm.me>
>> Date: Fri, 17 Mar 2023 14:45:58 +0000
>> Subject: [PATCH v3 1/3] Improve type handling in pg_dump's compress file API
> 
>> -int
>> +bool
>>  EndCompressFileHandle(CompressFileHandle *CFH)
>>  {
>> -	int			ret = 0;
>> +	bool		ret = 0;
> 
> Should say "= false" ?
> 

Right, fixed.

>>  	/*
>>  	 * Write 'size' bytes of data into the file from 'ptr'.
>> +	 *
>> +	 * Returns true on success and false on error.
>> +	 */
>> +       bool            (*write_func) (const void *ptr, size_t size,
> 
>> -        * Get a pointer to a string that describes an error that occurred during a
>> -        * compress file handle operation.
>> +        * Get a pointer to a string that describes an error that occurred during
>> +        * a compress file handle operation.
>>          */
>>         const char *(*get_error_func) (CompressFileHandle *CFH);
> 
> This should mention that the error accessible in error_func() applies (only) to
> write_func() ?
> 
> As long as this touches pg_backup_directory.c you could update the
> header comment to refer to "compressed extensions", not just .gz.
> 
> I noticed that EndCompressorLZ4() tests "if (LZ4cs)", but that should
> always be true.
> 

I haven't done these two things. We can/should do that, but it didn't
fit into the three patches.

> I was able to convert the zstd patch to this new API with no issue.
> 

Good to hear.


regards

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