RE: libpq compression - Mailing list pgsql-hackers

From Iwata, Aya
Subject RE: libpq compression
Date
Msg-id 71E660EB361DF14299875B198D4CE5423DEB752E@g01jpexmbkw25
Whole thread Raw
In response to Re: libpq compression  (Dmitry Dolgov <9erthalion6@gmail.com>)
Responses Re: libpq compression  (Dmitry Dolgov <9erthalion6@gmail.com>)
Re: libpq compression  (Konstantin Knizhnik <k.knizhnik@postgrespro.ru>)
List pgsql-hackers
Hi, 

> > I agree with the critiques from Robbie Harwood and Michael Paquier
> > that the way in that compression is being hooked into the existing
> > architecture looks like a kludge.  I'm not sure I know exactly how it
> > should be done, but the current approach doesn't look natural; it
> > looks like it was bolted on.
> 
> After some time spend reading this patch and investigating different points,
> mentioned in the discussion, I tend to agree with that. As far as I see it's
> probably the biggest disagreement here, that keeps things from progressing.
> I'm interested in this feature, so if Konstantin doesn't mind, I'll post in
> the near future (after I'll wrap up the current CF) an updated patch I'm working
> on right now to propose another way of incorporating compression. For now
> I'm moving patch to the next CF.

This thread seems to be stopped. 
In last e-mail, Dmitry suggest to update the patch that implements the function in another way, and as far as I saw, he
hasnot updated patch yet. (It may be because author has not responded.)
 
I understand big disagreement is here, however the status is "Needs review". 
There is no review after author update the patch to v9. So I will do.

About the patch, Please update your patch to attach current master. I could not test.

About Documentation, there are typos. Please check it. I am waiting for the reviewer of the sentence because I am not
sogood at English.
 

When you add new protocol message, it needs the information of "Length of message contents in bytes, including self.".

It provides supported compression algorithm as a Byte1. I think it better to provide it as a list like the
NegotiateProtocolVersionprotocol.
 

I quickly saw code changes.

+    nread = conn->zstream
+        ? zpq_read(conn->zstream, conn->inBuffer + conn->inEnd,
+                   conn->inBufSize - conn->inEnd, &processed)
+        : pqsecure_read(conn, conn->inBuffer + conn->inEnd,
+                        conn->inBufSize - conn->inEnd);

How about combine as a #define macro? Because there are same logic in two place.

Do you consider anything about memory control?
Typically compression algorithm keeps dictionary in memory. I think it needs reset or some method.


Regards,
Aya Iwata

pgsql-hackers by date:

Previous
From: Amit Langote
Date:
Subject: problems with foreign keys on partitioned tables
Next
From: Dmitry Dolgov
Date:
Subject: Re: libpq compression