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
