Hi Tomas, On Wed, Sep 11, 2024 at 8:08 PM Tomas Vondra <to...@vondra.me> wrote: > > 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. >
I didn't get the time to reply to you quickly, sorry about that. Thank you for improving the patch and appreciate your time for working on this. > > > Thanks for the report / patch. > > -- > Tomas Vondra -- Regards Junwang Zhao
