Re: row filtering for logical replication

On Mon, Nov 22, 2021 at 7:14 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Thu, Nov 18, 2021 at 12:33 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > PSA new set of v40* patches.
> >

I have a few more comments on 0007,


@@ -783,9 +887,28 @@ pgoutput_row_filter(PGOutputData *data, Relation
relation, HeapTuple oldtuple, H
             ExecDropSingleTupleTableSlot(entry->scantuple);
             entry->scantuple = NULL;
         }
+        if (entry->old_tuple != NULL)
+        {
+            ExecDropSingleTupleTableSlot(entry->old_tuple);
+            entry->old_tuple = NULL;
+        }
+        if (entry->new_tuple != NULL)
+        {
+            ExecDropSingleTupleTableSlot(entry->new_tuple);
+            entry->new_tuple = NULL;
+        }
+        if (entry->tmp_new_tuple != NULL)
+        {
+            ExecDropSingleTupleTableSlot(entry->tmp_new_tuple);
+            entry->tmp_new_tuple = NULL;
+        }

in pgoutput_row_filter, we are dropping the slots if there are some
old slots in the RelationSyncEntry.  But then I noticed that in
rel_sync_cache_relation_cb(), also we are doing that but only for the
scantuple slot.  So IMHO, rel_sync_cache_relation_cb(), is only place
setting entry->rowfilter_valid to false; so why not drop all the slot
that time only and in pgoutput_row_filter(), you can just put an
assert?

2.
+static bool
+pgoutput_row_filter_virtual(Relation relation, TupleTableSlot *slot,
RelationSyncEntry *entry)
+{
+    EState       *estate;
+    ExprContext *ecxt;


pgoutput_row_filter_virtual and pgoutput_row_filter are exactly same
except, ExecStoreHeapTuple(), so why not just put one check based on
whether a slot is passed or not, instead of making complete duplicate
copy of the function.

3.
         oldctx = MemoryContextSwitchTo(CacheMemoryContext);
         tupdesc = CreateTupleDescCopy(tupdesc);
         entry->scantuple = MakeSingleTupleTableSlot(tupdesc, &TTSOpsHeapTuple);

Why do we need to copy the tupledesc? do we think that we need to have
this slot even if we close the relation, if so can you add the
comments explaining why we are making a copy here.



-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com