RE: memory leak in pgoutput

On Saturday, November 16, 2024 4:37 PM by Yang <mobile.yang@outlook.com>  wrote:

> I recently noticed a unusually large memory consumption in pgoutput where
> "RelationSyncCache" is maintaining approximately 3 GB of memory while
> handling 15,000 tables.
> 
> Upon investigating the cause, I found that "tts_tupleDescriptor" in both
> "old_slot" and "new_slot" wouldn't be freed before the reusing the entry.
> The refcount of the tuple descriptor is initialized as -1 in init_tuple_slot(),
> which means it will not be refcounted. So, when cleaning the outdated slots,
> ExecDropSingleTupleTableSlot() would omit tuple descriptors.

Thanks for reporting the issue. I also confirmed that the bug exists and
your analysis is correct.

> 
> To address this issue, calling FreeTupleDesc() to release the tuple descriptor
> before ExecDropSingleTupleTableSlot() might be a suitable solution. However,
> I am uncertain whether this approach is appropriate.

I think the proposed change is reasonable. 

I considered another approach which is to mark tupledesc reference-counted
instead. But to make that work, we lack a global resource owner which is
required by IncrTupleDescRefCount/DecrTupleDescRefCount. (pgoutput doesn't
create its own resowner, only the toptransaction's resowner is available.)
Also, pgoutput doesn't reference the tupledesc in other places so it doesn't
seems useful to mark them reference-counted.

But I think there is an issue in the attached patch:

+			FreeTupleDesc(entry->old_slot->tts_tupleDescriptor);
 			ExecDropSingleTupleTableSlot(entry->old_slot);

Here, after freeing the tupledesc, the ExecDropSingleTupleTableSlot will still
access the freed tupledesc->tdrefcount which is an illegal memory access.

I think we can do something like below instead:

+			TupleDesc	desc = entry->old_slot->tts_tupleDescriptor;
+
+			Assert(desc->tdrefcount == -1);
+
 			ExecDropSingleTupleTableSlot(entry->old_slot);
+			FreeTupleDesc(desc);

Best Regards,
Hou zj