Re: libpq compression - Mailing list pgsql-hackers

On 24.11.2020 20:34, Daniil Zakhlystov wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, failed
> Implements feature:       tested, passed
> Spec compliant:           tested, failed
> Documentation:            tested, failed
>
> Submission review
> --
> Is the patch in a patch format which has context? (eg: context diff format)
> NO, need to fix
> Does it apply cleanly to the current git master?
> YES
> Does it include reasonable tests, necessary doc patches, etc?
> have docs, missing tests
>
> Usability review
> --
> At the moment, the patch supports per-connection (permanent) compression. The frontend can specify the desired
compressionalgorithms and compression levels and then negotiate the compression algorithm that is going to be used with
thebackend. In current state patch is missing the ability to enable/disable the compression on the backend side, I
thinkit might be not great from the usability side.
 
>
> Regarding on-the-fly configurable compression and different compression algorithms for each direction - these two
ideasare promising but tend to make the implementation more complex. However, the current implementation can be
extendedto support these approaches in the future. For example, we can specify switchable on-the-fly compression as
‘switchable’algorithm and negotiate it like the regular compression algorithm (like we currently negotiate ‘zstd’ and
‘zlib’).‘switchable’ algorithm may then introduce new specific messages to Postgres protocol to make the on-the-fly
compressionmagic work.
 
>
> The same applies to Robert’s idea of the different compression algorithms for different directions - we can introduce
itlater as a new compression algorithm with new specific protocol messages.
 
>
> Does the patch actually implement that?
> YES
>
> Do we want that?
> YES
>
> Do we already have it?
> NO
>
> Does it follow SQL spec, or the community-agreed behavior?
> To be discussed
>
> Does it include pg_dump support (if applicable)?
> not applicable
>
> Are there dangers?
> theoretically possible CRIME-like attack when using with SSL enabled
>
> Have all the bases been covered?
> To be discussed
>
>
> Feature test
> --
>
> I’ve applied the patch, compiled, and tested it with configure options --enable-cassert and --enable-debug turned on.
I’vetested the following scenarios:
 
>
> 1. make check
> =======================
>   All 201 tests passed.
> =======================
>
> 2. make check-world
> initially failed with:
> ============== running regression test queries        ==============
> test postgres_fdw                 ... FAILED     4465 ms
> ============== shutting down postmaster               ==============
> ======================
> 1 of 1 tests failed.
> ======================
> The differences that caused some tests to fail can be viewed in the
> file "/xxx/xxx/review/postgresql/contrib/postgres_fdw/regression.diffs".  A copy of the test summary that you see
aboveis saved in the file "/xxx/xxx/review/postgresql/contrib/postgres_fdw/regression.out".
 
>
> All tests passed after replacing ‘gsslib, target_session_attrs’ with ‘gsslib, compression, target_session_attrs’ in
line8914 of postgresql/contrib/postgres_fdw/expected/postgres_fdw.out
 
>
> 3. simple psql utility usage
> psql -d "host=xxx port=5432 dbname=xxx user=xxx compression=1"
>
> 4. pgbench tpcb-like w/ SSL turned ON
> pgbench "host=xxx port=5432 dbname=xxx user=xxx sslmode=require compression=1" --builtin tpcb-like -t 70 --jobs=32
--client=700
>
> 5. pgbench tpcb-like w/ SSL turned OFF
> pgbench "host=xxx port=5432 dbname=xxx user=xxx sslmode=disable compression=1" --builtin tpcb-like -t 70 --jobs=32
--client=700
>
> 6. pgbench initialization w/ SSL turned ON
> pgbench "host=xxx port=5432 dbname=xxx user=xxx sslmode=require compression=1" -i -s 500
>
> 7. pgbench initialization w/ SSL turned OFF
> pgbench "host=xxx port=5432 dbname=xxx user=xxx sslmode=disable compression=1" -i -s 500
>
> 8. Streaming physical replication. Recovery-related parameters
> recovery_target_timeline = 'latest'
> primary_conninfo = 'host=xxx port=5432 user=repl application_name=xxx compression=1'
> primary_slot_name = 'xxx'
> restore_command = 'some command'
>
> 9. This compression has been implemented in an experimental build of odyssey connection pooler and tested with ~1500
syntheticsimultaneous clients configuration and ~300 GB databases.
 
> During the testing, I’ve reported and fixed some of the issues.
>
> Does the feature work as advertised?
> YES
> Are there corner cases the author has failed to consider?
> NO
> Are there any assertion failures or crashes?
> NO
>
>
> Performance review
> --
>
> Does the patch slow down simple tests?
> NO
>
> If it claims to improve performance, does it?
> YES
>
> Does it slow down other things?
> Using compression may add a CPU overhead. This mostly depends on compression algorithm and chosen compression level.
Duringtesting with ZSTD algorithm and compression level 1 there was about 10% of CPU overhead in read/write balanced
scenariosand almost no overhead in mostly read scenarios.
 
>
>
> Coding review
> --
>
> In protocol.sgml:
>>         It can be just boolean values enabling or disabling compression
>>          ("true"/"false", "on"/"off", "yes"/"no", "1"/"0"), "auto" or explicit list of compression algorithms
>>          separated by comma with optional specification of compression level: "zlib,zstd:5".
> But in fe-protocol3.c:
>>     if (pg_strcasecmp(value, "true") == 0 ||
>>         pg_strcasecmp(value, "yes") == 0 ||
>>         pg_strcasecmp(value, "on") == 0 ||
>>         pg_strcasecmp(value, "any") == 0 ||
>>         pg_strcasecmp(value, "1") == 0)
>>     {
> I believe there is some mismatch - in docs, there is an “auto” parameter, but in code “auto” is missing, but “any”
exists.Actually, I propose to remove both “auto” and “any” parameters because they work the same way as “true/on/yes/1”
butappear like something else.
 
>
>
> In fe-protocol3.c:
>> #define pq_read_conn(conn)    \
>>     (conn->zstream        \
>>      ? zpq_read(conn->zstream, conn->inBuffer + conn->inEnd,            \
>>                 conn->inBufSize - conn->inEnd)                \
>>      : pqsecure_read(conn, conn->inBuffer + conn->inEnd,                \
>>                      conn->inBufSize - conn->inEnd))
> I think there should be some comment regarding the read function choosing logic. Same for zpq_write calls. Also,
pq_read_connis defined as a macros, but there is no macros for pq_write_conn.
 
>
> In configure.ac:
>> if test "$with_zstd" = yes; then
>>    AC_CHECK_LIB(zstd, ZSTD_decompressStream, [],
>>                 [AC_MSG_ERROR([zstd library not found
>> If you have zstd already installed, see config.log for details on the
>> failure.  It is possible the compiler isn't looking in the proper directory.
>> Use --without-zstd to disable zstd support.])])
>> fi
>> if test "$with_zstd" = yes; then
>>    AC_CHECK_HEADER(zstd.h, [], [AC_MSG_ERROR([zstd header not found
>> If you have zstd already installed, see config.log for details on the
>> failure.  It is possible the compiler isn't looking in the proper directory.
>> Use --without-zstd to disable zstd support.])])
>> fi
> Looks like the rows with --without-zstd are incorrect.
>
> In fe-connect.c:
>> if (index == (char)-1)
>> {
>>     appendPQExpBuffer(&conn->errorMessage,
>>     libpq_gettext(
>>     "server is not supported requested compression algorithms %s\n"),
>>     conn->compression);
>>     goto error_return;
>> }
> Right now this error might be displayed in two cases:
> Backend support compression, but it is somehow disabled/turned off
> Backend support compression, but does not support requested algorithms
>
> I think that it is a good idea to differentiate these two cases. Maybe define the following behavior somewhere in
docs:
>
> “When connecting to an older backend, which does not support compression, or in the case when the backend support
compressionbut for some reason wants to disable it, the backend will just ignore the _pq_.compression parameter and
won’tsend the compressionAck message to the frontend.” 
>
>
> To sum up, I think that the current implementation already introduces good benefits. As I proposed in the Usability
review,we may introduce the new approaches later as separate compression 'algorithms'.
 
>
> Thanks,
> Daniil Zakhlystov

Thank you for review.
New version of the patch addressing reported issues is attached.
I added libpq_comression GUC to be able to prohibit compression and 
server side if for some (security, high CPU load,..) reasons it is not 
desired.