On 20.06.2018 00:04, Robbie Harwood wrote:
Konstantin Knizhnik <k.knizh...@postgrespro.ru> writes:
On 18.06.2018 23:34, Robbie Harwood wrote:
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.
From my point of view compression of libpq traffic is similar with SSL
and should be toggled at the same stage.
But that's not what you're doing. This isn't where TLS gets toggled.
TLS negotiation happens as the very first packet: after completing the
TCP handshake, the client will send a TLS negotiation request. If it
doesn't happen there, it doesn't happen at all.
(You *could* configure it where TLS is toggled. This is, I think, not a
good idea. TLS encryption is a probe: the server can reject it, at
which point the client tears everything down and connects without TLS.
So if you do the same with compression, that's another point of tearing
down an starting over. The scaling on it isn't good either: if we add
another encryption method into the mix, you've doubled the number of
teardowns.)
Yes, you are right. There is special message for enabling TLS procotol.
But I do not think that the same think is needed for compression.
This is why I prefer to specify compression in connectoin options. So
compression may be enabled straight after processing of startup package.
Frankly speaking I still do no see reasons to postpone enabling
compression till some later moment.
Definitely authentication parameter are not so large to be efficiently
compressed, by compression (may be in future password protected) can
somehow protect this data.
In any case I do not think that compression of authentication data may
have any influence on negotiation speed.
So I am not 100% sure that toggling compression just after receiving
startup package is the only right solution.
But I am not also convinced in that there is some better place where
compressor should be configured.
Do you have some concrete suggestions for it? In InitPostgres just after
PerformAuthentication ?
Hmm, let me try to explain this differently.
pq_configure() (as you've called it) shouldn't send a packet. At its
callsite, we *already know* whether we want to use compression - that's
what the port->use_compression option says. So there's no point in
having a negotiation there - it's already happened.
My idea was the following: client want to use compression. But server
may reject this attempt (for any reasons: it doesn't support it, has no
proper compression library,
do not want to spend CPU for decompression,...) Right now compression
algorithm is hardcoded. But in future client and server may negotiate to
choose proper compression protocol.
This is why I prefer to perform negotiation between client and server to
enable compression.
The other thing you do in pq_configure() is call zpq_create(), which
does a bunch of initialization for you. I am pretty sure that all of
this can be deferred until the first time you want to send a compressed
message - i.e., when compress()/decompress() is called for the first
time from *secure_read() or *secure_write().
Also please notice that compression is useful not only for client-server
communication, but also for replication channels.
Right now it is definitely used in both cases, but if we move
pq_configure somewhere else, we should check that this code is invoked
in both for normal backends and walsender.
"We" meaning you, at the moment, since I don't think any of the rest of
us have set up tests with this code :)
If there's common code to be shared around, that's great. But it's not
imperative; in a lot of ways, the network stacks are very different from
each other, as I'm sure you've seen. Let's not have the desire for code
reuse get in the way of good, maintainable design.
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.
Sorry, but here I completely disagree with you.
What we are doing is really streaming compression, not compression of
individual messages/packages.
Yes, it is not a video, but actually COPY data has the same nature as
video data.
The main benefit of streaming compression is that we can use the same
dictionary for compressing all messages (and adjust this dictionary
based on new data).
We do not need to write dictionary and separate header for each record.
Otherwize compression of libpq messages will be completely useless:
typical size of message is too short to be efficiently compressed. The
main drawback of streaming compression is that you can not decompress
some particular message without decompression of all previous messages.
This is why streaming compression can not be used to compress database
pages (as it is done in CFS, provided in PostgresPro EE). But for libpq
it is no needed.
That makes sense, thanks. The zstd documentation doesn't articulate
that at all.
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).
Yes, it will simplify patch. But make libpq compression completely
useless (see my explanation above).
We need to use streaming compression, and to be able to use streaming
compression I have to pass function for fetching more data to
compression library.
I don't think you need that, even with the streaming API.
To make this very concrete, let's talk about ZSTD_decompressStream (I'm
pulling information from
https://facebook.github.io/zstd/zstd_manual.html#Chapter7 ).
Using the pseudocode I'm asking for above, the decompress() function
would look vaguely like this:
decompress(ptr, n)
ZSTD_inBuffer in = {0}
ZSTD_outBuffer out = {0}
in.src = ptr
in.size = n
while in.pos < in.size:
ret = ZSTD_decompressStream(out, in)
if ZSTD_isError(ret):
give_up()
memcpy(ptr, out.dst, out.pos)
return out.pos
(and compress() would follow a similar pattern, if we were to talk about
it).
It will not work in this way. We do not know how much input data we need
to be able to decompress message.
So loop should be something like this:
decompress(ptr, n)
ZSTD_inBuffer in = {0}
ZSTD_outBuffer out = {0}
in.src = ptr
in.size = n
while true
ret = ZSTD_decompressStream(out, in)
if ZSTD_isError(ret):
give_up()
if out.pos != 0
// if we deomcpress soemthing
return out.pos;
read_input(in);
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
