From: Robbie Harwood <rharwood@redhat.com>
To: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>,
 Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Grigory Smolkin <g.smolkin@postgrespro.ru>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: libpq compression
In-Reply-To: <f4ad0809-dd90-e321-5c1a-0ca68a136a8f@postgrespro.ru>
References: <aad16e41-b3f9-e89d-fa57-fb4c694bec25@postgrespro.ru>
 <dbf9035f-7f7b-b053-cc2a-52e5147d4557@postgrespro.ru>
 <429cf55b-a8bf-c4ba-196d-5b45420145a1@postgrespro.ru>
 <CAEepm=3CGUX4Vyw=jFaOedt99HS1HQiBKYTX4OYHo7HU2AczZg@mail.gmail.com>
 <36db5d02-45e6-b863-1418-825ef20ae4c6@postgrespro.ru>
 <CAEepm=3RAS8xVQK8rs+Xitzp+JXzGdGsMA7f+H8fjKQ7Do8aSA@mail.gmail.com>
 <f4ad0809-dd90-e321-5c1a-0ca68a136a8f@postgrespro.ru>
Date: Mon, 18 Jun 2018 16:34:43 -0400
Message-ID: <jlg4lhzdeak.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Precedence: bulk

--=-=-=
Content-Type: text/plain

t
--=-=-=
Content-Type: multipart/signed; boundary="==-=-=";
	micalg=pgp-sha512; protocol="application/pgp-signature"

--==-=-=
Content-Type: text/plain

Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:

> On 06.06.2018 02:03, Thomas Munro wrote:
>> On Wed, Jun 6, 2018 at 2:06 AM, Konstantin Knizhnik
>> <k.knizhnik@postgrespro.ru> 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

--==-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEA5qc6hnelQjDaHWqJTL5F2qVpEIFAlsoF2MACgkQJTL5F2qV
pEIDlA/8DLWLZUOyMAKoWgRZc8baV4LFlGxQV5zBwid1boyP4OlMsU/RiaanL9+Z
i0RWShiEQtvJaGx14T5C1udgrCgTTE7SpSN9uFF60SBQ6IBtIISw2kJziFOiKfzH
X5XLSfAJalIDh3CcHJie6zfvkbWnah6AXaCIKR9zViJ2x2uE0dPji/7TVi9VBC4d
PBXJUj5FIW3txNyMpoFYBvDWbnb8dx2FAFsmWhNhdfipfRlvmTb9FZVNGBluiI9C
i1hfVtRHT6P6FvfvrIdMOEUOltlrkkW6PblsPtqUHp/2qS/CqWnhGytCpbe3h5br
zBUYpS38PsFvmnmhpECvEHLQK8jyPPu4fZ3Mcfb07a4q/kIp4eInWc1lB+xrQltJ
9A7WjCbJU7GxxxNVMhoz3aDt/UwM8MBobGEoAgdoEXWKLOS7jEC4W9J7lBG8WOk6
xpiWTXYkA4NpTdF5X+SY/7/gdDfFwrUMmmKi0dCQr7y18HT+pmdr/ytzW0InW7OO
Q4k/te3i2y4520zK8ZyDRoSuRfyPHLi814rWYIEGv/0M9eq0LB8Y3k6ncluJ6+Tu
gqrL1tzlzrUHj4duOgAigJmhUO1VQxYnQzNC1MzvjC2J9hamboFtYfoKWv8sXxw/
/J2Q0XsE0uNf4/UH+cNPapemOmNp3Q1WJLOOJgaF18Y9gtstSjs=
=CE14
-----END PGP SIGNATURE-----
--==-=-=--

--=-=-=--