Message-ID: <1963a93f-001d-aa60-dec8-a7e1c6e03203@enterprisedb.com>
Date: Mon, 8 May 2023 20:20:42 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: Add LZ4 compression in pg_dump
Content-Language: en-US
To: gkokolatos@pm.me, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>,
 Andrew Dunstan <andrew@dunslane.net>, Alexander Lakhin
 <exclusion@gmail.com>, Justin Pryzby <pryzby@telsasoft.com>,
 shiy.fnst@fujitsu.com, pgsql-hackers@lists.postgresql.org,
 Rachel Heaton <rachelmheaton@gmail.com>
References: 
 <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
 <3beb0dd7-c927-3eca-689f-716e255d5b27@enterprisedb.com>
 <94ae9bca-5ebb-1e68-bb7b-4f32e89fefbe@gmail.com>
 <MmuCeC4k017VCWzr-f8MEQ9hx8DRak2ZGbb5SVwKvBBHqCt8tLKzVVdMcPMsi3Zj314OJbs1UPEzwc5RwOv3IMp1lAkC7rg-yDOaDBJGww8=@pm.me>
 <71868915-0345-1b7b-f3d6-ec66547c662a@dunslane.net>
 <P-Dl7-QRVPmjv35H2n-Wnj9amDG_MUEb0-jtlzs05n5z03lq0IZNTsI5h4DjTmLekfmKexgFBQhmVXtUO402vAIFx0e002lIzsb8O8Hi1wA=@pm.me>
 <ZFhCyn4Gm2eu60rB@paquier.xyz> <219662.1683508165@sss.pgh.pa.us>
 <220484.1683508611@sss.pgh.pa.us>
 <SYTRcNgtAbzyn3y3IInh1x-UfNTKMNpnFvI3mr6SyqyVf3PkaDsMy_cpKKgsl3_HdLy2MFAH4zwjxDmFfiLO8rWtSiJWBtqT06OMjeNo4GA=@pm.me>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <SYTRcNgtAbzyn3y3IInh1x-UfNTKMNpnFvI3mr6SyqyVf3PkaDsMy_cpKKgsl3_HdLy2MFAH4zwjxDmFfiLO8rWtSiJWBtqT06OMjeNo4GA=@pm.me>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/1963a93f-001d-aa60-dec8-a7e1c6e03203%40enterprisedb.com>
Precedence: bulk


On 5/8/23 18:19, gkokolatos@pm.me wrote:
> 
> 
> 
> 
> 
> ------- Original Message -------
> On Monday, May 8th, 2023 at 3:16 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> 
>>
>>
>> I wrote:
>>
>>> Michael Paquier michael@paquier.xyz writes:
>>>
>>>> While testing this patch, I have triggered an error pointing out that
>>>> the decompression path of LZ4 is broken for table data. I can
>>>> reproduce that with a dump of the regression database, as of:
>>>> make installcheck
>>>> pg_dump --format=d --file=dump_lz4 --compress=lz4 regression
>>
>>> Ugh. Reproduced here ... so we need an open item for this.
>>
>>
>> BTW, it seems to work with --format=c.
>>
> 
> Thank you for the extra tests. It seems that exists a gap in the test
> coverage. Please find a patch attached that is addressing the issue
> and attempt to provide tests for it.
> 

Seems I'm getting messages with a delay - this is mostly the same fix I
ended up with, not realizing you already posted a fix.

I don't think we need the local "in" variable - the pointer parameter is
local in the function, so we can modify it directly (with a cast).
WriteDataToArchiveLZ4 does it that way too.

The tests are definitely a good idea. I wonder if we should add a
comment to DEFAULT_IO_BUFFER_SIZE mentioning that if we choose to
increase the value in the future, we needs to tweak the tests too to use
more data in order to exercise the buffering etc. Maybe it's obvious?


regards

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