Hi,
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).
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.
Can you please clarify your opinion about connection poolers? Do you mean by sensible way that in some cases they can just forward the compressed stream without parsing?
+/*
+ * Index of used compression algorithm in zpq_algorithms array.
+ */
+static int zpq_algorithm_impl;
This is just odd API design imo. Why doesn't the dispatch work based on
an argument for zpq_create() and the ZpqStream * for the rest?
What if there's two libpq connections in one process? To servers
supporting different compression algorithms? This isn't going to fly.
I agree, I think that moving the zpq_algorithm_impl to the ZpqStream struct seems like an easy fix for this issue.
+ /* 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.
Maybe we can just split the PqStream into PqCompressStream and PqDecompressStream? It looks like that they can work independently.
—
Daniil Zakhlystov
