On Fri, 6 Mar 2020 at 08:54, Petr Jelinek <[email protected]> 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?
>
> We could also merge the binary and changed arrays into single char array
> named something like format (as data can be either unchanged, binary or
> text) and just reuse same identifiers we have in protocol.
>
This seems like a good idea.
Dave Cramer
>
> --
> Petr Jelinek
> 2ndQuadrant - PostgreSQL Solutions for the Enterprise
> https://www.2ndQuadrant.com/
>
