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/
