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(at)2ndquadrant(dot)com
> <mailto:petr(at)2ndquadrant(dot)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/