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