On 9/10/24 21:47, Tomas Vondra wrote: > ... > > The only question that bothers me a little bit is the possibility of a > memory leak - could it happen that we keep the copied key much longer > than needed? Or does aggcontext have with the right life span? AFAICS > that's where we allocate the aggregate state, so it seems fine. > > Also, how far back do we need to backpatch this? ITSM PG15 does not have > this issue, and it was introduced with the SQL/JSON stuff in PG16. Is > that correct? >
Nah, I spent a bit of time looking for a memory leak, but I don't think there's one, or at least not a new one. We use the same memory context as for the hash table / buffer, so that should be fine. But this made me realize the code in json_build_object_worker() can simply use pstrdup() to copy the key into CurrentMemoryContext, which is where the hash table of unique keys is. In fact, using unique_check.mcxt would not be quite right: MemoryContext mcxt; /* context for saving skipped keys */ And this has nothing to do with skipped keys. So I adjusted that way and pushed. Thanks for the report / patch. -- Tomas Vondra
