Subject: RE: memory leak in pgoutput

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 18 Nov 2024 02:53:52 +0000

On Saturday, November 16, 2024 4:37 PM by Yang <[email-masked]>  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

---

From: by Yang <[email-masked]>
Date: Mon, 18 Nov 2024 07:00:57 +0000

> access the freed tupledesc->tdrefcount which is an illegal memory access.

Yes, I overlooked that.

>                         ExecDropSingleTupleTableSlot(entry->old_slot);

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().

```
+                                              desc = NULL;
```

By the way, this issue is introduced in 52e4f0cd472d39d. Therefore, we may need
to backport the patch to v15.

Best Regards,
Boyu Yang