Re: row filtering for logical replication

On Sat, Jan 29, 2022 at 6:01 AM Andres Freund <andres@anarazel.de> wrote:
>
>
> > +     if (has_filter)
> > +     {
> > +             /* Create or reset the memory context for row filters */
> > +             if (entry->cache_expr_cxt == NULL)
> > +                     entry->cache_expr_cxt = AllocSetContextCreate(CacheMemoryContext,
> > +
"Rowfilter expressions",
 
> > +
ALLOCSET_DEFAULT_SIZES);
> > +             else
> > +                     MemoryContextReset(entry->cache_expr_cxt);
>
> I see this started before this patch, but I don't think it's a great idea that
> pgoutput does a bunch of stuff in CacheMemoryContext. That makes it
> unnecessarily hard to debug leaks.
>
> Seems like all this should live somwhere below ctx->context, allocated in
> pgoutput_startup()?
>

Agreed.

> Consider what happens in a long-lived replication connection, where
> occasionally there's a transient error causing streaming to stop. At that
> point you'll just loose all knowledge of entry->cache_expr_cxt, no?
>

I think we will lose knowledge because the WALSender exits on ERROR
but that would be true even when we allocate it in this new allocated
context. Am, I missing something?

-- 
With Regards,
Amit Kapila.