Message-ID: <d62901d9-e945-049f-5b0e-7f645d83a1b9@enterprisedb.com>
Date: Thu, 6 Apr 2023 17:34:30 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: zstd compression for pg_dump
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Jacob Champion <jchampion@timescale.com>, pgsql-hackers@postgresql.org,
 gkokolatos@pm.me, Michael Paquier <michael@paquier.xyz>,
 Robert Haas <robertmhaas@gmail.com>, Peter Geoghegan <pg@bowt.ie>,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 Heikki Linnakangas <hlinnaka@iki.fi>, Thomas Munro <thomas.munro@gmail.com>,
 Dipesh Pandit <dipesh.pandit@gmail.com>,
 Andrey Borodin <x4mmm@yandex-team.ru>,
 Mark Dilger <mark.dilger@enterprisedb.com>
References: <ZCHSSv3DgxUa0f3L@telsasoft.com>
 <0022dd9e-a439-d72f-d09b-ec512c23949b@enterprisedb.com>
 <ZCdpzywVGl0BLan2@telsasoft.com>
 <4029fe58-f661-7d8d-cbbb-b936a05531e2@enterprisedb.com>
 <ZCd6sb7XA/W6nLaD@telsasoft.com>
 <2de3bcad-6da9-9022-bce5-fc0416351d6e@enterprisedb.com>
 <ZCgzbBVKLvwpqMzT@telsasoft.com>
 <f3947107-a95a-348c-263c-284ac7275ae0@enterprisedb.com>
 <ZCsmSoiBMVfrlOXm@telsasoft.com>
 <27eb89fc-8fdd-2988-0959-0af1d6d7a12a@enterprisedb.com>
 <ZCuTotQ38nwA/hLY@telsasoft.com>
 <51013008-6735-96a2-d744-3b0a0413fcdd@enterprisedb.com>
In-Reply-To: <51013008-6735-96a2-d744-3b0a0413fcdd@enterprisedb.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/d62901d9-e945-049f-5b0e-7f645d83a1b9%40enterprisedb.com>
Precedence: bulk


On 4/5/23 21:42, Tomas Vondra wrote:
> On 4/4/23 05:04, Justin Pryzby wrote:
>> On Mon, Apr 03, 2023 at 11:26:09PM +0200, Tomas Vondra wrote:
>>> On 4/3/23 21:17, Justin Pryzby wrote:
>>>> On Sat, Apr 01, 2023 at 10:26:01PM +0200, Tomas Vondra wrote:
>>>>>> Feel free to mess around with threads (but I'd much rather see the patch
>>>>>> progress for zstd:long).
>>>>>
>>>>> OK, understood. The long mode patch is pretty simple. IIUC it does not
>>>>> change the format, i.e. in the worst case we could leave it for PG17
>>>>> too. Correct?
>>>>
>>>> Right, libzstd only has one "format", which is the same as what's used
>>>> by the commandline tool.  zstd:long doesn't change the format of the
>>>> output: the library just uses a larger memory buffer to allow better
>>>> compression.  There's no format change for zstd:workers, either.
>>>
>>> OK. I plan to do a bit more review/testing on this, and get it committed
>>> over the next day or two, likely including the long mode. One thing I
>>> noticed today is that maybe long_distance should be a bool, not int.
>>> Yes, ZSTD_c_enableLongDistanceMatching() accepts int, but it'd be
>>> cleaner to cast the value during a call and keep it bool otherwise.
>>
>> Thanks for noticing.  Evidently I wrote it using "int" to get the
>> feature working, and then later wrote the bool parsing bits but never
>> changed the data structure.
>>
>> This also updates a few comments, indentation, removes a useless
>> assertion, and updates the warning about zstd:workers.
>>
> 
> Thanks. I've cleaned up the 0001 a little bit (a couple comment
> improvements), updated the commit message and pushed it. I plan to take
> care of the 0002 (long distance mode) tomorrow, and that'll be it for
> PG16 I think.

I looked at the long mode patch again, updated the commit message and
pushed it. I was wondering if long_mode should really be bool -
logically it is, but ZSTD_CCtx_setParameter() expects int. But I think
that's fine.

I think that's all for PG16 in this patch series. If there's more we
want to do, it'll have to wait for PG17 - Justin, can you update and
submit the patches that you think are relevant for the next CF?


regards

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