Re: libpq compression

On 28.10.2020 22:58, Alvaro Herrera wrote:
> On 2020-Oct-26, Konstantin Knizhnik wrote:
>
>> +    while (bufptr < bufend || zpq_buffered(PqStream) != 0) /* has more data to flush or unsent data in internal
compressionbuffer */
 
>>       {
>> -        int            r;
>> -
>> -        r = secure_write(MyProcPort, bufptr, bufend - bufptr);
>> -
>> -        if (r <= 0)
>> +        int        r;
>> +        size_t  processed = 0;
>> +        size_t  available = bufend - bufptr;
>> +        r = PqStream
>> +            ? zpq_write(PqStream, bufptr, available, &processed)
>> +            : secure_write(MyProcPort, bufptr, available);
>> +        bufptr += processed;
>> +        PqSendStart += processed;
> This bit is surprising to me.  I thought the whole zpq_write() thing
> should be hidden inside secure_write, so internal_flush would continue
> to call just secure_write; and it is that routine's responsibility to
> call zpq_write or be_tls_write or secure_raw_write etc according to
> compile-time options and socket state.
Sorry, may be it is not the nicest way of coding.
Ideally, we should use "decorator design pattern" here where each layer 
(compression, TLS,...) is implemented as separate decorator class.
This is how io streams are implemented in java and many other SDKs.
But it requires  too much redesign of Postgres code.

Also from my point of view the core of the problem is that in Postgres 
there are two almost independent implementation of networking code for 
backend/frontend.
IMHO it is better to have some SAL (system abstraction layer) which 
hides OS specific stuff from rest of the system and which can be shared 
by backend and frontend.

In any case I failed to implement it in different way.
The basic requirements was:
1. zpq code should be used both by backend and frontent.
2. decompressor may need to perform multiple reads from underlying layer 
to fill its buffer and be able to produce some output.
3. minimize changes in postgres code
4. be able to use zpq library in some other tools (like pooler)

This is why zpq_create function is given tx/rx functions to pefrom IO 
operations.
secure_write is such tx function for backend (and pqsecure_write for 
frontend).

Actually the name of this function secure_write assumes that it deals 
only with TLS, not with compression.
Certainly it is possible to rename it or better introduce some other 
functions, i.e. stream_read which will perform this checks.
But please notice that it is not enough to perform all checks in one 
functions as you suggested.  It should be really pipe each component of 
which is doing its own job:
encryption, compression....

As for me I prefer to have in this place indirect function calls.
But there are several reasons (at least different prototypes of the 
functions)
which makes me choose the current way.

In any case: there are many different ways of doing the same task.
And different people have own opinion about best/right way of doing it.
Definitely there are some objective criteria: encapsulation, lack of 
code duplication,
readability of code,...

I tried to find the best approach base on my own preferences and 
requirements described above.
May be I am wrong but then I want to be convinced that suggested 
alternative is better.
 From my point of view calling compressor from function named 
secure_read is not right solution...



-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company