On Tue, Dec 21, 2021 at 6:17 AM Ajin Cherian <itsa...@gmail.com> wrote: > > On Tue, Dec 21, 2021 at 5:58 AM Euler Taveira <eu...@eulerto.com> wrote: > > > >In pgoutput_row_filter_update(), first, we are deforming the tuple in > >local datum, then modifying the tuple, and then reforming the tuple. > >I think we can surely do better here. Currently, you are reforming > >the tuple so that you can store it in the scan slot by calling > >ExecStoreHeapTuple which will be used for expression evaluation. > >Instead of that what you need to do is to deform the tuple using > >tts_values of the scan slot and later call ExecStoreVirtualTuple(), so > >advantages are 1) you don't need to reform the tuple 2) the expression > >evaluation machinery doesn't need to deform again for fetching the > >value of the attribute, instead it can directly get from the value > >from the virtual tuple. > > Storing the old tuple/new tuple in a slot and re-using the slot avoids > the overhead of > continuous deforming of tuple at multiple levels in the code. >
Yeah, deforming tuples again can have a significant cost but what is the need to maintain tmp_new_tuple in relsyncentry. I think that is required in rare cases, so we can probably allocate/deallocate when required. Few other comments: ================== 1. TupleTableSlot *scantuple; /* tuple table slot for row filter */ + TupleTableSlot *new_tuple; /* slot for storing deformed new tuple during updates */ + TupleTableSlot *old_tuple; /* slot for storing deformed old tuple during updates */ I think it is better to name these as scan_slot, new_slot, old_slot to avoid confusion with tuples. 2. +++ b/src/backend/replication/logical/proto.c @@ -19,6 +19,7 @@ #include "replication/logicalproto.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "executor/executor.h" The include is in wrong order. We keep includes in alphabatic order. 3. @@ -832,6 +847,7 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, bool binar ReleaseSysCache(typtup); } + } Spurious addition. 4. -logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, bool binary) +logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple, TupleTableSlot *slot, +bool binary) The formatting is quite off. Please run pgindent. 5. If we decide to go with this approach then I feel let's merge the required comments from Euler's version. -- With Regards, Amit Kapila.