Re: libpq compression - Mailing list pgsql-hackers

On Nov 1, 2020, at 12:08 AM, Konstantin Knizhnik <k.knizhnik@postgrespro.ru> wrote:

Hi

On 31.10.2020 00:03, Andres Freund wrote:

Hi,

On 2020-10-29 16:45:58 +0300, Konstantin Knizhnik wrote:

- What does " and it is up to the client whether to continue work
   without compression or report error" actually mean for a libpq parameter?

It can not happen.
The client request from server use of compressed protocol only if
"compression=XXX" was specified in connection string.
But XXX should be supported by client, otherwise this request will be
rejected.
So supported protocol string sent by client can never be empty.

I think it's pretty important for features like this to be able to fail
softly when the feature is not available on the other side. Otherwise a
lot of applications need to have unnecessary version dependencies coded
into them.

Sorry, may be I do not completely understand your suggestion.
Right now user jut specify that he wants to use compression.
Libpq client sends to the server list of supported algorithms
and server choose among them the best one is supported.
It sends it chose to the client and them are both using this algorithm.

Sorry, that in previous mail I have used incorrect samples: client is not explicitly specifying compression algorithm -
it just request compression. And server choose the most efficient algorithm which is supported both by client and server.
So client should not know names of the particular algorithms (i.e. zlib, zstd) and
choice is based on the assumption that server (or better say programmer)
knows better than user which algorithms is more efficient.
Last assumption me be contested because user better know which content will be send and which
algorithm is more efficient for this content. But right know when the choice is between zstd and zlib,
the first one is always better: faster and provides better quality.

- What is the point of the "streaming mode" reference?

There are ways of performing compression:
- block mode when each block is individually compressed (compressor stores
dictionary in each compressed blocked)
- stream mode
Block mode allows to independently decompress each page. It is good for
implementing page or field compression (as pglz is used to compress toast
values).
But it is not efficient for compressing client-server protocol commands.
It seems to me to be important to explain that libpq is using stream mode
and why there is no pglz compressor

To me that seems like unnecessary detail in the user oriented parts of
the docs at least.

Ok, I will remove this phrase.

Why are compression methods identified by one byte identifiers? That
seems unnecessarily small, given this is commonly a once-per-connection
action?

It is mostly for simplicity of implementation: it is always simple to work
with fixed size messages (or with array of chars rather than array of
strings).
And I do not think that it can somehow decrease flexibility: this one-letter
algorihth codes are not visible for user. And I do not think that we
sometime will support more than 127 (or even 64 different compression
algorithms).

It's pretty darn trivial to have a variable width protocol message,
given that we have all the tooling for that. Having a variable length
descriptor allows us to extend the compression identifier with e.g. the
level without needing to change the protocol. E.g. zstd:9 or
zstd:level=9 or such.

I suggest using a variable length string as the identifier, and split it
at the first : for the lookup, and pass the rest to the compression
method.

Yes, I agree that it provides more flexibility.
And it is not a big problem to handle arbitrary strings instead of chars.
But right now my intention was to prevent user from choosing compression algorithm and especially specifying compression level
(which are algorithm-specific). Its matter of server-client handshake to choose best compression algorithm supported by both of them.
I can definitely rewrite it, but IMHO given to much flexibility for user will just complicate things without any positive effect.

The protocol sounds to me like there's no way to enable/disable
compression in an existing connection. To me it seems better to have an
explicit, client initiated, request to use a specific method of
compression (including none). That allows to enable compression for bulk
work, and disable it in other cases (e.g. for security sensitive
content, or for unlikely to compress well content).

It will significantly complicate implementation (because of buffering at
different levels).

Really? We need to be able to ensure a message is flushed out to the
network at precise moments - otherwise a lot of stuff will break.

Definitely stream is flushed after writing each message.
The problem is at receiver side: several messages can be sent without waiting response and will be read into the buffer. If first is compressed
and subsequent - not, then it will be not so trivial to handle it. I have already faced with this problem when compression is switched on:
backend may send some message right after acknowledging compression protocol.  This message is already compressed but is delivered together with uncompressed compression acknowledgement message. I have solved this problem in switching on compression at client side,
but really do not want to solve it at arbitrary moments. And once again: my opinion is that too much flexibility is not so good here:
there is no sense to switch off compression for short messages (overhead of switching can be larger than compression itself).

Also it is not clear to me who and how will control enabling/disabling
compression in this case?
I can imagine that "\copy" should trigger compression. But what about
(server side) "copy"  command?

The client could still trigger that. I don't think it should ever be
server controlled.

Sorry, but I still think that possibility to turn compression on/off on the fly is very doubtful idea.
And it will require extension of protocol (adding some extra functions to libpq library to control it).

And concerning security risks... In most cases such problem is not relevant
at all because both client and server are located within single reliable
network.

The trend is towards never trusting the network, even if internal, not
the opposite.

It if security of communication really matters, you should not switch
compression in all cases (including COPY and other bulk data transfer).
It is very strange idea to let client to decide which data is "security
sensitive" and which not.

Huh?

Sorry, if I was unclear. I am not specialist in security.
But it seems to be very dangerous when client (and not server) decides which data is "security sensitive"
and which not. Just an example: you have VerySecreteTable. And when you perform selects on this table,
you switch compression off. But then client want to execute COPY command for this table and as far as it requires bulk data transfer,
user decides to switch on compression.

Actually specking about security risks has no sense: if you want to provide security, then you use TLS. And it provides its own compression.
So there is no need to perform compression at libpq level. libpq comression is for the cases when you do not need SSL at all.

I think that would also make cross-version handling easier, because a
newer client driver can send the compression request and handle the
error, without needing to reconnect or such.

Most importantly, I think such a design is basically a necessity to make
connection poolers to work in a sensible way.

I do not completely understand the problem with connection pooler.
Right now developers of Yandex Odyssey are trying to support libpq
compression in their pooler.
If them will be faced with some problems, I will definitely address
them.

It makes poolers a lot more expensive if they have to decompress and
then recompress again. It'd be awesome if even the decompression could
be avoided in at least some cases, but compression is the really
expensive part. So if a client connects to the pooler with
compression=zlib and an existing server connection is used, the pooler
should be able to switch the existing connection to zlib.

My expectation was that pooler is installed in the same network as database and there is no need
to compress traffic between pooler and backends.

But even if it is really needed I do not see problems here.
Definitely we need zpq library to support different compression algorithms in the same application.
But it is done.

To avoid pooler perform decompression+compression ist is necessary to send command header in raw format.
But it will greatly reduce effect of stream compression.

But if you think that it is so important, I will try to implement it. Many
questions arise in this case: which side should control compression level?
Should client affect compression level both at client side and at server
side? Or it should be possible to specify separately compression level for
client and for server?

I don't think the server needs to know about what the client does,
compression level wise.

Sorry, I do not understand you.
Compression takes place at  sender side.
So both client compressing its messages before sending to server
and server compresses responses sent to the client.
In both cases compression level may be specified.
And it is not clear whether client should control compression level of the server.

+<para>
+        Used compression algorithm. Right now the following streaming compression algorithms are supported: 'f' - Facebook zstd, 'z' - zlib, 'n' - no compression.
+</para>

I would prefer this just be referenced as zstd or zstandard, not
facebook zstd. There's an RFC (albeit only "informational"), and it
doesn't name facebook, except as an employer:
https://tools.ietf.org/html/rfc8478

Please notice that it is internal encoding, user will specify
psql -d "dbname=postgres compression=zstd"

If name "zstd" is not good, I can choose any other.

All I was saying is that I think you should not name it ""Facebook zstd",
just "zstd".

Sorry, it was my error.
Right now compression name is not specified by user at all: just boolean value on/off.

I think you should just use something like

pq_beginmessage(buf, 'z');
pq_sendbyte(buf, compression_method_byte);
pq_endmessage(buf);

like most of the backend does? And if you, as I suggest, use a variable
length compression identifier, use
pq_sendcountedtext(buf, compression_method, strlen(compression_method));
or such.

Sorry, I can not do it in this way because pq connection with client is not yet initialized.
So I can not use pq_endmessage here and have to call secure_write manually.

I am not saying it's necessarily the best approach, but it doesn't seem
like it'd be hard to have zpq not send / receive the data from the
network, but just transform the data actually to-be-sent/received from
the network.

E.g. in pq_recvbuf() you could do something roughly like the following,
after a successful secure_read()

    if (compression_enabled)
    {
        zpq_decompress(network_data, network_data_length,
                       &input_processed, &output_data, &output_data_length);
        copy_data_into_recv_buffer(output_data, output_data_length);
        network_data += input_processed;
        network_data_length -= input_processed;
    }

Sorry, it is not possible because to produce some output, compressor may need to perform multiple read.
And doing it in two different places, i.e. in pqsecure_read and secure_read is worse than doing it on once place - s it is done now.

and the inverse on the network receive side. The advantage would be that
we can then more easily use zpq stuff in other places.

If we do go with the current callback model, I think we should at least
extend it so that a 'context' parameter is shuffled through zpq, so that
other code can work without global state.

Callback is has void* arg where arbitrary daat can be passed to callback.

+ r = PqStream
+ ? zpq_read(PqStream, PqRecvBuffer + PqRecvLength,
+    PQ_RECV_BUFFER_SIZE - PqRecvLength, &processed)
+ : secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
+   PQ_RECV_BUFFER_SIZE - PqRecvLength);
+ PqRecvLength += processed;

? : doesn't make sense to me in this case. This should be an if/else.

Isn't it a matter of style preference?
Why  if/else is principle better than ?:

?: makes sense when it's either much shorter, or when it allows to avoid
repeating a bunch of code. E.g. if it's just a conditional argument to a
function with a lot of other arguments.

But when, as here, it just leads to weirder formatting, it really just
makes the code harder to read.

From my point of view the main role of ?: construction is not just saving some bytes/lines of code.
It emphasizes that both parts of conditional construction produce the same result.
As in this case it is result of read operation. If you replace it with if-else, this relation between actions done in two branches is missed
(or at least less clear).

I agree that sometimes ?: leads to more complex and obscure expressions.
But to you think that if-else in this case will lead to more clear or
readable code?

Yes, pretty clearly for me.

    r = PqStream
        ? zpq_read(PqStream, PqRecvBuffer + PqRecvLength,
                   PQ_RECV_BUFFER_SIZE - PqRecvLength, &processed)
        : secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
                   PQ_RECV_BUFFER_SIZE - PqRecvLength);
vs

    if (PqStream)
        r = zpq_read(PqStream, PqRecvBuffer + PqRecvLength,
                     PQ_RECV_BUFFER_SIZE - PqRecvLength, &processed);
    else
        r = secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
                        PQ_RECV_BUFFER_SIZE - PqRecvLength);

the if / else are visually more clearly distinct, the sole added
repetition is r =. And most importantly if / else are more common.

I agree that ?: is less clear than if/else. In most of modern programming languages there is no construction ?:
but if-else can be used as expression. And repetition is always bad, isn't it? Not just because of writing extra code,
but because of possible inconsistencies and errors (just like normal forms in databases).

In any case - I do not think that it is principle: if you prefer - I can replace it with if-else

Another question is whether conditional expression here is really good idea.
I prefer to replace with indirect function call...

A branch is cheaper than indirect calls, FWIW>

Really? Please notice that we do not compare just branch with indirect call, but branch+direct call vs. indirect call.
I didn't measure it. But in any case IMHO performance aspect is less important here than clearance and maintainability of code.
And indirect call is definitely better in this sense... Unfortunately I can not use it (certainly it is not absolutely impossible, but it requires much
more changes).

+#define ZSTD_BUFFER_SIZE (8*1024)
+#define ZSTD_COMPRESSION_LEVEL 1

Add some arguments for choosing these parameters.

What are the suggested way to specify them?
I can not put them in GUCs (because them are also needed at client side).

I don't mean arguments as in configurable, I mean that you should add a
comment explaining why 8KB / level 1 was chosen.

Sorry, will do.

+/*
+ * Get list of the supported algorithms.
+ * Each algorithm is identified by one letter: 'f' - Facebook zstd, 'z' - zlib.
+ * Algorithm identifies are appended to the provided buffer and terminated by '\0'.
+ */
+void
+zpq_get_supported_algorithms(char algorithms[ZPQ_MAX_ALGORITHMS])
+{
+ int i;
+ for (i = 0; zpq_algorithms[i].name != NULL; i++)
+ {
+ Assert(i < ZPQ_MAX_ALGORITHMS);
+ algorithms[i] = zpq_algorithms[i].name();
+ }
+ Assert(i < ZPQ_MAX_ALGORITHMS);
+ algorithms[i] = '\0';
+}

Uh, doesn't this bake ZPQ_MAX_ALGORITHMS into the ABI? That seems
entirely unnecessary?

I tried to avoid use of dynamic memory allocation because zpq is used both
in client and server environments with different memory allocation
policies.

We support palloc in both(). But even without that, you can just have
one function to get the number of algorithms, and then have the caller
pass in a large enough array.

Certainly it is possible.
But why complicate implementation if it is internal function used only in single place in the code?

And I afraid that using the _pq_ parameter stuff makes enabling of
compression even less user friendly.

I didn't intend to suggest that the _pq_ stuff should be used in a
client facing manner. What I suggesting is that if the connection string
contains compression=, the driver would internally translate that to
_pq_.compression to pass that on to the server, which would allow the
connection establishment to succeed, even if the server is a bit older.

New client can establish connection with old server if it is not using compression.
And if it wants to use compression, then _pq_.compression will not help much.
Old server will ignore it (unlike compression= option) but then client will wait for acknowledgement from
server for used  compression algorithm and didn't receive one. Certainly it is possible to handle this situation (if we didn't
receive expected message) but why it is better than adding compression option to startup package?
But if you think that adding new options to startup package should be avoided as much as possible,
then I will replace it with _pq_.compression.

Greetings,

Andres Freund

Thank you very much for review and explanations.