Thread: 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
> Here, after freeing the tupledesc, the ExecDropSingleTupleTableSlot will still
> access the freed tupledesc->tdrefcount which is an illegal memory access.
Yes, I overlooked that.
> 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);
It seems a bit odd because "entry->old_slot->tts_tupleDescriptor" is accessed
after "entry->old_slot" has been freed. I think we can avoid this by assigning
"desc" to NULL before ExecDropSingleTupleTableSlot().
```
+ TupleDesc desc = entry->old_slot->tts_tupleDescriptor;
+
+ Assert(desc->tdrefcount == -1);
+
+ FreeTupleDesc(desc);
+ desc = NULL;
ExecDropSingleTupleTableSlot(entry->old_slot);
```
By the way, this issue is introduced in 52e4f0cd472d39d. Therefore, we may need
to backport the patch to v15.
Best Regards,
Boyu Yang