Thread: RE: memory leak in pgoutput

RE: memory leak in pgoutput

From
"Zhijie Hou (Fujitsu)"
Date:
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

Re: memory leak in pgoutput

From
by Yang
Date:

> 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