Re: libpq compression

On Nov 1, 2020, at 3:01 PM, Konstantin Knizhnik <k.knizhnik@postgrespro.ru> wrote:

Hi

On 01.11.2020 12:37, Daniil Zakhlystov wrote:

Hi,

I have a couple of comments regarding the last patch, mostly these are minor issues.

In src/backend/libpq/pqcomm.c, starting from the line 1114:

int

pq_getbyte_if_available(unsigned char *c)

{

int r;

Assert(PqCommReadingMsg);

if (PqRecvPointer < PqRecvLength || (0) > 0)   // not easy to understand optimization (maybe add a comment?)

{

*c = PqRecvBuffer[PqRecvPointer++];

return 1;

}

return r;             // returned value is not initialized

}

Sorry, I don't understand it.
This is the code of pq_getbyte_if_available:

int
pq_getbyte_if_available(unsigned char *c)
{
    int            r;

    Assert(PqCommReadingMsg);

    if (PqRecvPointer < PqRecvLength || (r = pq_recvbuf(true)) > 0)
    {
        *c = PqRecvBuffer[PqRecvPointer++];
        return 1;
    }
    return r;
}


So "return r" branch is executed when both conditions are false: (PqRecvPointer < PqRecvLength)
and ((r = pq_recvbuf(true)) > 0)
Last condition cause assignment of "r" variable.

I wonder how did you get this "returned value is not initialized" warning?
Is it produced by some static analyze tool or compiler?

In any case, I will initialize "r" variable to make compiler happy.

In src/interfaces/libpq/fe-connect.c, starting from the line 3255:

pqGetc(&algorithm, conn);

impl = zpq_get_algorithm_impl(algorithm);

{ // I believe that if (impl < 0) condition is missing here, otherwise there is always an error

   appendPQExpBuffer(&conn->errorMessage,

                 libpq_gettext(

                                                "server is not supported requested compression algorithm %c\n"), algorithm);

   goto error_return;

}

Sorry I have fixed this mistyping several days ago in GIT repository
 git@github.com:postgrespro/libpq_compression.git
but did;t attach new version of the patch because I plan to make more changes as a result of Andres review.

In configure, starting from the line 1587:

--without-zlib          do not use Zlib

--with-zstd             do not use zstd // is this correct?

Thank you for noting it: fixed.

<libpq-compression-22.patch>