On Tue, Aug 2, 2022 at 1:53 PM Jacob Champion <jchamp...@timescale.com> wrote: > > and changing the status to "Needs Review"
I've tried the patch, it works as advertised. The code generally is OK, maybe some functions require comments (because at least their neighbours have some). Some linkers complain about zs_is_valid_impl_id() being inline while used in other modules. Some architectural notes: 1. Currently when the user requests compression from libpq, but the server does not support any of the codecs client have - the connection will be rejected by client. I think this should be configured akin to SSL: on, try, off. 2. On the zpq_stream level of abstraction we parse a stream of bytes to look for individual message headers. I think this is OK, just a little bit awkward. 3. CompressionAck message can be completely replaced by ParameterStatus message. 4. Instead of sending a separate SetCompressionMethod, the CompressedData can have its header with the index of the used method and the necessity to restart compression context. 5. Portions of pg_stat_network_traffic can be extracted to separate patch step to ease the review. And, actually, the scope of this view is slightly beyond compression anyway. What do you think? Also, compression is a very cool and awaited feature, hope to see it committed one day, thank you for working on this! Best regards, Andrey Borodin.
