Re: Binary support for pgoutput plugin

On Fri, 3 Apr 2020 at 03:43, Petr Jelinek <petr@2ndquadrant.com> wrote:

> 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.
>

OK, I can see an easy way to only allocate the number of elements required
but since OidReceiveFunctionCall takes
StringInfo as one of it's arguments it seems like an easy path unless there
is something I am missing ?

Dave