Konstantin Knizhnik writes: > On 06.06.2018 02:03, Thomas Munro wrote: >> On Wed, Jun 6, 2018 at 2:06 AM, Konstantin Knizhnik >> wrote: >>> Thank you for review. Updated version of the patch fixing all reported >>> problems is attached. >> Small problem on Windows[1]: >> >> C:\projects\postgresql\src\include\common/zpq_stream.h(17): error >> C2143: syntax error : missing ')' before '*' >> [C:\projects\postgresql\libpq.vcxproj] >> 2395 >> >> You used ssize_t in zpq_stream.h, but Windows doesn't have that type. >> We have our own typedef in win32_port.h. Perhaps zpq_stream.c should >> include postgres.h/postgres_fe.h (depending on FRONTEND) like the >> other .c files in src/common, before it includes zpq_stream.h? >> Instead of "c.h". >> >> [1] https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.1106 >> > Thank you very much for reporting the problem. > I attached new patch with include of postgres_fe.h added to zpq_stream.c Hello! Due to being in a similar place, I'm offering some code review. I'm excited that you're looking at throughput on the network stack - it's not usually what we think of in database performance. Here are some first thoughts, which have some overlap with what others have said on this thread already: ### This build still doesn't pass Windows: https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.2277 You can find more about what the bot is doing here: http://cfbot.cputube.org/ ### I have a few misgivings about pq_configure(), starting with the name. The *only* thing this function does is set up compression, so it's mis-named. (There's no point in making something generic unless it's needed - it's just confusing.) I also don't like that you've injected into the *startup* path - before authentication takes place. Fundamentally, authentication (if it happens) consists of exchanging some combination of short strings (e.g., usernames) and binary blobs (e.g., keys). None of this will compress well, so I don't see it as worth performing this negotiation there - it can wait. It's also another message in every startup. I'd leave it to connection parameters, personally, but up to you. ### Documentation! You're going to need it. There needs to be enough around for other people to implement the protocol (or if you prefer, enough for us to debug the protocol as it exists). In conjunction with that, please add information on how to set up compressed vs. uncompressed connections - similarly to how we've documentation on setting up TLS connection (though presumably compressed connection documentation will be shorter). ### Using terminology from https://facebook.github.io/zstd/zstd_manual.html : Right now you use streaming (ZSTD_{compress,decompress}Stream()) as the basis for your API. I think this is probably a mismatch for what we're doing here - we're doing one-shot compression/decompression of packets, not sending video or something. I think our use case is better served by the non-streaming interface, or what they call the "Simple API" (ZSTD_{decompress,compress}()). Documentation suggests it may be worth it to keep an explicit context around and use that interface instead (i.e., ZSTD_{compressCCTx,decompressDCtx}()), but that's something you'll have to figure out. You may find making this change helpful for addressing the next issue. ### I don't like where you've put the entry points to the compression logic: it's a layering violation. A couple people have expressed similar reservations I think, so let me see if I can explain using `pqsecure_read()` as an example. In pseudocode, `pqsecure_read()` looks like this: if conn->is_tls: n = tls_read(conn, ptr, len) else: n = pqsecure_raw_read(conn, ptr, len) return n I want to see this extended by your code to something like: if conn->is_tls: n = tls_read(conn, ptr, len) else: n = pqsecure_raw_read(conn, ptr, len) if conn->is_compressed: n = decompress(ptr, n) return n In conjunction with the above change, this should also significantly reduce the size of the patch (I think). ### The compression flag has proven somewhat contentious, as you've already seen on this thread. I recommend removing it for now and putting it in a separate patch to be merged later, since it's separable. ### It's not worth flagging style violations in your code right now, but you should be aware that there are quite a few style and whitespace problems. In particular, please be sure that you're using hard tabs when appropriate, and that line lengths are fewer than 80 characters (unless they contain error messages), and that pointers are correctly declared (`void *arg`, not `void* arg`). ### Thanks, --Robbie