Re: Binary support for pgoutput plugin

On Sun, 27 Oct 2019 at 11:00, Dmitry Dolgov <9erthalion6(at)gmail(dot)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