Re: libpq compression

On 05.06.2018 08:26, Thomas Munro wrote:
> On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
> <k(dot)knizhnik(at)postgrespro(dot)ru> wrote:
>> Thank you for this notice.
>> Updated and rebased patch is attached.
> Hi Konstantin,
>
> Seems very useful. +1.
>
> + rc = inflate(&zs->rx, Z_SYNC_FLUSH);
> + if (rc != Z_OK)
> + {
> + return ZPQ_DECOMPRESS_ERROR;
> + }
>
> Does this actually guarantee that zs->rx.msg is set to a string? I
> looked at some documentation here:
>
> https://www.zlib.net/manual.html
>
> It looks like return value Z_DATA_ERROR means that msg is set, but for
> the other error codes Z_STREAM_ERROR, Z_BUF_ERROR, Z_MEM_ERROR it
> doesn't explicitly say that. From a casual glance at
> https://github.com/madler/zlib/blob/master/inflate.c I think it might
> be set to Z_NULL and then never set to a string except in the mode =
> BAD paths that produce the Z_DATA_ERROR return code. That's
> interesting because later we do this:
>
> + if (r == ZPQ_DECOMPRESS_ERROR)
> + {
> + ereport(COMMERROR,
> + (errcode_for_socket_access(),
> + errmsg("Failed to decompress data: %s", zpq_error(PqStream))));
> + return EOF;
>
> ... where zpq_error() returns zs->rx.msg. That might crash or show
> "(null)" depending on libc.
>
> Also, message style: s/F/f/
>
> +ssize_t zpq_read(ZpqStream* zs, void* buf, size_t size, size_t* processed)
>
> Code style: We write "Type *foo", not "Type* var". We put the return
> type of a function definition on its own line.
>
> It looks like there is at least one place where zpq_stream.o's symbols
> are needed but it isn't being linked in, so the build fails in some
> ecpg stuff reached by make check-world:
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -fexcess-precision=standard -g -O2 -pthread -D_REENTRANT
> -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS test1.o
> -L../../../../../src/port -L../../../../../src/common -L../../ecpglib
> -lecpg -L../../pgtypeslib -lpgtypes
> -L../../../../../src/interfaces/libpq -lpq -Wl,--as-needed
> -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags -lpgcommon
> -lpgport -lpthread -lz -lreadline -lrt -lcrypt -ldl -lm -o test1
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_free'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_error'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_read'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to
> `zpq_buffered'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to
> `zpq_create'
> ../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_write'
>

Hi Thomas,
Thank you for review. Updated version of the patch fixing all reported
problems is attached.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company