Re: libpq compression - Mailing list pgsql-hackers

**sorry for the noise, but I need to re-send the message because one of the recipients is blocked on the pgsql-hackers
forsome reason** 

Hi!

Done, the patch should apply to the current master now.

Actually, I have an almost-finished version of the patch with the previously requested asymmetric compression
negotiation.I plan to attach it soon. 


Thanks,

Daniil Zakhlystov


> On 14 Jul 2021, at 16:37, vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, Apr 22, 2021 at 6:34 PM Daniil Zakhlystov
> <usernamedt@yandex-team.ru> wrote:
>>
>>> On 21 Apr 2021, at 20:35, Ian Zagorskikh <izagorskikh@cloudlinux.com> wrote:
>>>
>>> Let me drop my humble 2 cents in this thread. At this moment I checked only
0001-Add-zlib-and-zstd-streaming-compressionpatch. With no order: 
>>>
>>> * No checks for failures. For example, value from malloc() is not checked for NULL and used as-is right after the
call.Also as a result possibly NULL values passed into ZSTD functions which are explicitly not NULL-tolerant and so
dereferencepointers without checks. 
>>>
>>> * Used memory management does not follow the schema used in the common module. Direct calls to std C malloc/free
arehard coded. AFAIU this is not backend friendly. Looking at the code around I believe they should be wrapped to
ALLOC/FREElocal macroses that depend on FRONTEND define. As it's done for example. in src/common/hmac.c. 
>>>
>>> * If we're going to fix our code to be palloc/pfree friendly there's also a way to pass our custom allocator
callbacksinside ZSTD functions. By default ZSTD uses malloc/free but it can be overwritten by the caller in e.g.
ZSTD_createDStream_advanced(ZSTD_customMemcustomMem) versions of API . IMHO that would be good. If a 3rd party
componentallows us to inject  a custom memory allocator and we do have it why not use this feature? 
>>>
>>> * Most zs_foo() functions do not check ptr arguments, though some like zs_free() do. If we're speaking about a
"commonAPI" that can be used by a wide range of different modules around the project, such a relaxed way to treat input
argumentsIMHO can be an evil. Or at least add Assert(ptr) assertions so they can be catched up in debug mode. 
>>>
>>> * For zs_create() function which must be called first to create context for further operations, a compression
methodis passed as integer. This method is used inside zs_create() as index inside an array of compress implementation
descriptors.There are several problems: 
>>> 1) Method ID value is not checked for validity. By passing an invalid method ID we can easily get out of array
bounds.
>>> 2) Content of array depends on on configuration options e.g. HAVE_LIBZSTD, HAVE_LIBZ etc. So index inside this
arrayis not persistent. For some config options combination method ID with value 0 may refer to ZSTD and for others to
zlib.
>>> 3) There's no defines/enums/etc in public z_stream.h header that would define which value should be passed to
createa specific compressor. Users have to guess it or check the code. 
>>>
>>> * I have a feeling after reading comments for ZSTD compress/decompress functions that their return value is treated
ina wrong way handling only success path. Though need more checks/POCs on that. 
>>>
>>> In general, IMHO the idea of a generic compress/decompress API is very good and useful. But specific implementation
needssome code cleanup/refactoring. 
>>
>> Hi,
>>
>> thank you for the detailed review. Previously in the thread, Justin Pryzby mentioned the preliminary patch which
willbe common between the different compression patches. Basically, this is a quick prototype of that patch and it
definitelyneeds some additional effort. It would be also great to have a top-level overview of the
0001-Add-zlib-and-zstd-streaming-compressionpatch from Justin to make sure it can be potentially be used for the other
compressionpatches. 
>>
>> Currently, I’m in the process of implementing the negotiation of asymmetric compression, but I can take a look at
theseissues after I finish with it. 
>>
>>> One little fix to 0001-Add-zlib-and-zstd-streaming-compression patch for configure.ac
>>>
>>> ================
>>> @@ -1455,6 +1456,7 @@ fi
>>>
>>> if test "$with_lz4" = yes; then
>>>  AC_CHECK_HEADERS(lz4.h, [], [AC_MSG_ERROR([lz4.h header file is required for LZ4])])
>>> +fi
>>> ================
>>>
>>> Otherwise autoconf generates a broken configure script.
>>
>> Added your fix to the patch and rebased it onto the current master.
>
> The patch does not apply on Head anymore, could you rebase and post a
> patch. I'm changing the status to "Waiting for Author".
>
> Regards,
> Vignesh