From: | "Iwata, Aya" <iwata(dot)aya(at)jp(dot)fujitsu(dot)com> |
---|---|
To: | 'Dmitry Dolgov' <9erthalion6(at)gmail(dot)com>, Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "rharwood(at)redhat(dot)com" <rharwood(at)redhat(dot)com>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, "g(dot)smolkin(at)postgrespro(dot)ru" <g(dot)smolkin(at)postgrespro(dot)ru>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: libpq compression |
Date: | 2019-01-09 10:25:28 |
Message-ID: | 71E660EB361DF14299875B198D4CE5423DEB752E@g01jpexmbkw25 |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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 has not 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 so good 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 NegotiateProtocolVersion protocol.
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
From | Date | Subject | |
---|---|---|---|
Next Message | Dmitry Dolgov | 2019-01-09 10:44:09 | Re: libpq compression |
Previous Message | Amit Langote | 2019-01-09 10:21:38 | problems with foreign keys on partitioned tables |