Subject: Re: zstd compression for pg_dump
To: Daniil Zakhlystov <usernamedt@yandex-team.ru>,
 Justin Pryzby <pryzby@telsasoft.com>, Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers <pgsql-hackers@postgresql.org>
References: <20201221194924.GI30237@telsasoft.com>
 <554920.1608580960@sss.pgh.pa.us> <20201222023235.GJ30237@telsasoft.com>
 <20210104025321.GA9712@telsasoft.com>
 <E3868F55-750B-407A-8C15-6C790B5D4D77@yandex-team.ru>
 <94586A5E-6A18-4EB7-B646-5249EC6FE266@yandex-team.ru>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <3866daa6-cc68-2dbe-e7db-0478f1c3d21a@enterprisedb.com>
Date: Fri, 12 Mar 2021 23:31:57 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <94586A5E-6A18-4EB7-B646-5249EC6FE266@yandex-team.ru>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/3866daa6-cc68-2dbe-e7db-0478f1c3d21a%40enterprisedb.com>
Precedence: bulk


On 1/4/21 11:17 AM, Daniil Zakhlystov wrote:
> Hi!
> 
>> On Jan 4, 2021, at 11:04 AM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>>
>> Daniil, is levels definition compatible with libpq compression patch?
>> +typedef struct Compress {
>> +	CompressionAlgorithm	alg;
>> +	int			level;
>> +	/* Is a nondefault level set ?  This is useful since different compression
>> +	 * methods have different "default" levels.  For now we assume the levels
>> +	 * are all integer, though.
>> +	*/
>> +	bool		level_set;
>> +} Compress;
> 
> Similarly to this patch, it is also possible to define the compression level at the initialization stage in libpq compression patch.
> 
> The difference is that in libpq compression patch the default compression level always equal to 1, independently of the chosen compression algorithm.
> 
>> On Jan 4, 2021, at 11:04 AM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>>
>> Libpq compression encountered some problems with memory consumption which required some extra config efforts.
> 
> 
>> On Jan 4, 2021, at 12:06 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
>>
>> RAM use is not significantly different from zlib, except that zstd --long adds
>> more memory.
> 
> Regarding ZSTD memory usage:
> 
> Recently I’ve made a couple of tests of libpq compression with different ZLIB/ZSTD compression levels which shown that compressing/decompressing ZSTD w/ high compression levels 
> require to allocate more virtual (Commited_AS) memory, which may be exploited by malicious clients:
> 
> https://www.postgresql.org/message-id/62527092-16BD-479F-B503-FA527AF3B0C2%40yandex-team.ru
> 
> We can avoid high memory usage by limiting the max window size to 8MB. This should effectively disable the support of compression levels above 19:
> https://www.postgresql.org/message-id/6A45DFAA-1682-4EF2-B835-C5F46615EC49%40yandex-team.ru
> 
> So maybe it is worthwhile to use similar restrictions in this patch.
>

I think there's a big difference between those two patches. In the libpq
case, the danger is that the client requests the server to compress the
data in a way that requires a lot of memory. I.e. the memory is consumed
on the server.

With this pg_dump patch, the compression is done by the pg_dump process,
not the server. So if the attacker configures the compression in a way
that requires a lot of memory, so what? He'll just allocate memory on
the client machine, where he could also just run a custom binary that
does a huge malloc().

So I don't think we need to worry about this too much.


regards

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