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