Re: libpq compression

On 08.02.2019 10:01, Iwata, Aya wrote:
> Hi,
>
> I am sorry for my late reply.
>
>> I fixed all issues you have reported except using list of supported compression
>> algorithms.
> Sure. I confirmed that.
>
>> It will require extra round of communication between client and server to
>> make a decision about used compression algorithm.
> In beginning of this thread, Robbie Harwood said that no extra communication needed.
> I think so, too.

Well, I think that this problem is more complex and requires more
discussion.
There are three places determining choice of compression algorithm:
1. Specification of compression algorithm by client. Right now it is
just boolean "compression" parameter in connection string,
but it is obviously possible to specify concrete algorithm here.
2. List of compression algorithms supported by client.
3. List of compression algorithms supported by server.

Concerning first option I have very serious doubt that it is good idea
to let client choose compression protocol.
Without extra round-trip it can be only implemented in this way:
if client toggles compression option in connection string, then libpq
includes in startup packet list of supported compression algorithms.
Then server intersects this list with its own set of supported
compression algorithms and if result is not empty, then
somehow choose  one of the commonly supported algorithms and sends it to
the client with 'z' command.

One more question: should we allow custom defined compression methods
and if so, how them can be handled at client side (at server we can use
standard extension
dynamic loading mechanism).

Frankly speaking, I do not think that such flexibility in choosing
compression algorithms is really needed.
I do not expect that there will be many situations where old client has
to communicate with new server or visa versa.
In most cases both client and server belongs to the same postgres
distributive and so implements the same compression algorithm.
As far as we are compressing only temporary data (traffic), the problem
of providing backward compatibility seems to be not so important.

>
>> I still not sure whether it is good idea to make it possible to user to
>> explicitly specify compression algorithm.
>> Right now used streaming compression algorithm is hardcoded and depends on
>> --use-zstd ort --use-zlib configuration options.
>> If client and server were built with the same options, then they are able
>> to use compression.
> I understand that compression algorithm is hardcoded in your proposal.
> However given the possibility of future implementation, I think
> it would be better for it to have a flexibility to choose compression library.
>
> src/backend/libpq/pqcomm.c :
> In current Postgres source code, pq_recvbuf() calls secure_read()
> and pq_getbyte_if_available() also calls secure_read().
> It means these functions are on the same level.
> However in your change, pq_getbyte_if_available() calls pq_recvbuf(),
> and pq_recvbuf() calls secure_read(). The level of these functions is different.
>
> I think the purpose of pq_getbyte_if_available() is to get a character if it exists and
> the purpose of pq_recvbuf() is to acquire data up to the expected length.
> In your change, pq_getbyte_if_available() may have to do unnecessary process waiting or something.

Sorry, but this change is essential. We can have some available data in
compression buffer and we need to try to fetch it in
pq_getbyte_if_available()
instead of just returning EOF.
>
> So how about changing your code like this?
> The part that checks whether it is compressed is implemented as a #define macro(like fe_misc.c). And pq_recvbuf() and pq_getbyte_if_available() modify little, like this;
>
> - r = secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
> - PQ_RECV_BUFFER_SIZE - PqRecvLength);
> + r = SOME_DEFINE_NAME_();
>
> configure:
> Adding following message to the top of zlib in configure
> ```
> {$as_echo "$as_me:${as_lineno-$LINENO}:checking whethere to build with zstd support">&5
> $as_echo_n "checking whether to build with zstd suppor... ">&6;}
> ```

Sorry, but it seems to me that the following fragment of configure is
doing it:

+
+if test "$with_zstd" = yes ; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for ZSTD_compress
in -lzstd" >&5
+$as_echo_n "checking for ZSTD_compress in -lzstd... " >&6; }
+if ${ac_cv_lib_zstd_ZSTD_compress+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_check_lib_save_LIBS=$LIBS
+LIBS="-lzstd  $LIBS"
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char ZSTD_compress ();
+int
+main ()
+{
+return ZSTD_compress ();
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_lib_zstd_ZSTD_compress=yes
+else
+  ac_cv_lib_zstd_ZSTD_compress=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+LIBS=$ac_check_lib_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$ac_cv_lib_zstd_ZSTD_compress" >&5
+$as_echo "$ac_cv_lib_zstd_ZSTD_compress" >&6; }
+if test "x$ac_cv_lib_zstd_ZSTD_compress" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_LIBZSTD 1
+_ACEOF
+
+  LIBS="-lzstd $LIBS"
+
+else
+  as_fn_error $? "library 'zstd' is required for ZSTD support" "$LINENO" 5
+fi
+
+fi

> Regards,
> Aya Iwata

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