Re: libpq compression

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.

> > - 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.

> > 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.

> > 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.

> 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.

> 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?

> > 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.

> 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.

> > > +<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".

> > > + if (client_compression_algorithms)
> > > + {
> > > + char server_compression_algorithms[ZPQ_MAX_ALGORITHMS];
> > > + char compression_algorithm = ZPQ_NO_COMPRESSION;
> > > + char compression[6] = {'z',0,0,0,5,0}; /* message length = 5 */
> > > + int rc;
> > Why is this hand-rolling protocol messages?
> Sorry, I do not quite understand your concern.
> It seems to me that all libpq message manipulation is more or less
> hand-rolling, isn't it (we are not using protobuf or msgbpack)?
> Or do you think that  calling pq_sendbyte,pq_sendint32,... is much safer in
> this case?

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.

> > > + socket_set_nonblocking(false);
> > > + while ((rc = secure_write(MyProcPort, compression, sizeof(compression))) < 0
> > > + && errno == EINTR);
> > > + if ((size_t)rc != sizeof(compression))
> > > + return -1;
> > Huh? This all seems like an abstraction violation.
> >
> >
> > > + /* initialize compression */
> > > + if (zpq_set_algorithm(compression_algorithm))
> > > + PqStream = zpq_create((zpq_tx_func)secure_write, (zpq_rx_func)secure_read, MyProcPort);
> > > + }
> > > + return 0;
> > > +}
> > Why is zpq a wrapper around secure_write/read? I'm a bit worried this
> > will reduce the other places we could use zpq.

> zpq has to read/write data from underlying stream.
> And it should be used both in client and server environment.
> I didn't see other ways  to provide single zpq implementation without code
> duplication
> except pass to it rx/tx functions.

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;
}

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.

> > > + 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.

> 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.

> 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>

> > > +#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.

> > > +/*
> > > + * 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.

> 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.

Greetings,

Andres Freund