RE: memory leak in pgoutput

On Saturday, November 16, 2024 4:37 PM by Yang <mobile(dot)yang(at)outlook(dot)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