Re: Add LZ4 compression in pg_dump

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(at)pm(dot)me wrote:
>> From a174cdff4ec8aad59f5bcc7e8d52218a14fe56fc Mon Sep 17 00:00:00 2001
>> From: Georgios Kokolatos <gkokolatos(at)pm(dot)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