Re: row filtering for logical replication

On Tue, Dec 21, 2021 at 6:17 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Tue, Dec 21, 2021 at 5:58 AM Euler Taveira <euler(at)eulerto(dot)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.