Re: libpq compression

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)
> {
> intr;
>
> 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(at)github(dot)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.