Hi Tomas,
On Wed, Sep 11, 2024 at 8:08 PM Tomas Vondra <tomas@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
