Re: Binary support for pgoutput plugin

Hi,

On 08/03/2020 00:18, Dave Cramer wrote:
> On Fri, 6 Mar 2020 at 08:54, Petr Jelinek <petr@2ndquadrant.com 
> <mailto:petr@2ndquadrant.com>> wrote:
> 
>     Hi Dave,
> 
>     On 29/02/2020 18:44, Dave Cramer wrote:
>      >
>      >
>      > rebased and removed the catversion bump.
> 
>     Looked into this and it generally seems okay, but I do have one
>     gripe here:
> 
>      > +                                     tuple->values[i].data =
>     palloc(len + 1);
>      > +                                     /* and data */
>      > +
>      > +                                     pq_copymsgbytes(in,
>     tuple->values[i].data, len);
>      > +                                     tuple->values[i].len = len;
>      > +                                     tuple->values[i].cursor = 0;
>      > +                                     tuple->values[i].maxlen = len;
>      > +                                     /* not strictly necessary
>     but the docs say it is required */
>      > +                                     tuple->values[i].data[len]
>     = '\0';
>      > +                                     break;
>      > +                             }
>      > +                     case 't':                       /* text
>     formatted value */
>      > +                             {
>      > +                                     tuple->changed[i] = true;
>      > +                                     int len = pq_getmsgint(in,
>     4);  /* read length */
>      >
>      >                                       /* and data */
>      > -                                     tuple->values[i] =
>     palloc(len + 1);
>      > -                                     pq_copymsgbytes(in,
>     tuple->values[i], len);
>      > -                                     tuple->values[i][len] = '\0';
>      > +                                     tuple->values[i].data =
>     palloc(len + 1);
>      > +                                     pq_copymsgbytes(in,
>     tuple->values[i].data, len);
>      > +                                     tuple->values[i].data[len]
>     = '\0';
>      > +                                     tuple->values[i].len = len;
> 
>     The cursor should be set to 0 in the text formatted case too if this is
>     how we chose to encode data.
> 
>     However I am not quite convinced I like the StringInfoData usage here.
>     Why not just change the struct to include additional array of lengths
>     rather than replacing the existing values array with StringInfoData
>     array, that seems generally both simpler and should have smaller memory
>     footprint too, no?
> 
> 
> Can you explain this a bit more? I don't really see a huge difference in 
> memory usage.
> We still need length and the data copied into LogicalRepTupleData when 
> sending the data in binary, no?
> 

Yes but we don't need to have fixed sized array of 1600 elements that 
contains maxlen and cursor positions of the StringInfoData struct which 
we don't use for anything AFAICS.


-- 
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/