Re: Binary support for pgoutput plugin - Mailing list pgsql-hackers

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