Re: Column Filtering in Logical Replication

The code in get_rel_sync_entry() changes current memory context to
CacheMemoryContext, then does a bunch of memory-leaking things.  This is
not good, because that memory context has to be very carefully managed
to avoid permanent memory leaks.  I suppose you added that because you
need something -- probably entry->att_map -- to survive memory context
resets, but if so then you need to change to CacheMemoryContext only
when that memory is allocated, not other chunks of memory.  I suspect
you can fix this by moving the MemoryContextSwitchTo() to just before
calling get_table_columnset; then all the leaky thinkgs are done in
whatever the original memory context is, which is fine.

(However, you also need to make sure that ->att_map is carefully freed
at the right time.  It looks like this already happens in
rel_sync_cache_relation_cb, but is rel_sync_cache_publication_cb
correct?  And in get_rel_sync_entry() itself, what if the entry already
has att_map -- should it be freed prior to allocating another one?)

By the way, I notice that your patch doesn't add documentation changes,
which are of course necessary.



/me is left wondering about PGOutputData->publication_names memory
handling ...

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/