On Thu, Nov 18, 2021 at 11:02 AM Peter Smith <smithpb2...@gmail.com> wrote: > > On Mon, Nov 15, 2021 at 9:31 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > 4. I think we should add some comments in pgoutput_row_filter() as to > > why we are caching the row_filter here instead of > > get_rel_sync_entry()? That has been discussed multiple times so it is > > better to capture that in comments. > > Added comment in v40 [1] >
I think apart from truncate and error cases, it can also happen for other operations because we decide whether to publish a change (operation) after calling get_rel_sync_entry() in pgoutput_change. I think we can reflect that as well in the comment. > > > > 5. Why do you need a separate variable rowfilter_valid to indicate > > whether a valid row filter exists? Why exprstate is not sufficient? > > Can you update comments to indicate why we need this variable > > separately? > > I have improved the (existing) comment in v40 [1]. > One more thing related to this code: pgoutput_row_filter() { .. + if (!entry->rowfilter_valid) { .. + oldctx = MemoryContextSwitchTo(CacheMemoryContext); + tupdesc = CreateTupleDescCopy(tupdesc); + entry->scantuple = MakeSingleTupleTableSlot(tupdesc, &TTSOpsHeapTuple); + MemoryContextSwitchTo(oldctx); .. } Why do we need to initialize scantuple here unless we are sure that the row filter is going to get associated with this relentry? I think when there is no row filter then this allocation is not required. -- With Regards, Amit Kapila.