Re: Binary support for pgoutput plugin

On Sun, 27 Oct 2019 at 11:00, Dmitry Dolgov <9erthalion6@gmail.com> wrote:

> > On Mon, Jun 17, 2019 at 10:29:26AM -0400, Dave Cramer wrote:
> > > Which is what I have done. Thanks
> > >
> > > I've attached both patches for comments.
> > > I still have to add documentation.
> >
> > Additional patch for documentation.
>
> Thank you for the patch! Unfortunately 0002 has some conflicts, could
> you please send a rebased version? In the meantime I have few questions:
>
>     -LogicalRepRelId
>     +void
>      logicalrep_read_insert(StringInfo in, LogicalRepTupleData *newtup)
>      {
>         char            action;
>     -   LogicalRepRelId relid;
>     -
>     -   /* read the relation id */
>     -   relid = pq_getmsgint(in, 4);
>
>         action = pq_getmsgbyte(in);
>         if (action != 'N')
>     @@ -175,7 +173,6 @@ logicalrep_read_insert(StringInfo in,
> LogicalRepTupleData *newtup)
>
>         logicalrep_read_tuple(in, newtup);
>
>     -   return relid;
>      }
>
>     ....
>
>     -   relid = logicalrep_read_insert(s, &newtup);
>     +   /* read the relation id */
>     +   relid = pq_getmsgint(s, 4);
>         rel = logicalrep_rel_open(relid, RowExclusiveLock);
>     +
>     +   logicalrep_read_insert(s, &newtup);
>
> Maybe I'm missing something, what is the reason of moving pq_getmsgint
> out of logicalrep_read_*? Just from the patch it seems that the code is
> equivalent.
>
> > There is one obvious hack where in binary mode I reset the input
> > cursor to allow the binary input to be re-read From what I can tell
> > the alternative is to convert the data in logicalrep_read_tuple but
> > that would require moving a lot of the logic currently in worker.c to
> > proto.c. This seems minimally invasive.
>
> Which logic has to be moved for example? Actually it sounds more natural
> to me, if this functionality would be in e.g. logicalrep_read_tuple
> rather than slot_store_data, since it has something to do with reading
> data. And it seems that in pglogical it's also located in
> pglogical_read_tuple.
>

Ok, I've rebased and reverted logicalrep_read_insert

As to the last comment, honestly it's been so long I can't remember why I
put that comment in there.

Thanks for reviewing

Dave