On Thu, 13 Jul 2023 at 17:20, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Matthias van de Meent <boekewurm+postg...@gmail.com> writes: > > My collegue Konstantin (cc-ed) noticed that the GiST code of intarray > > may leak memory in certain index operations: > > Can you demonstrate an actual problem here, that is a query-lifespan leak? > > IMO, the coding rule in the GiST and GIN AMs is that the AM code is > responsible for running all opclass-supplied functions in suitably > short-lived memory contexts, so that leaks within those functions > don't cause problems. This is different from btree which requires > comparison functions to not leak. The rationale for having different > conventions is that btree comparison functions are typically simple > enough to be able to deal with such a restriction, whereas GiST > and GIN opclasses are often complex critters for which it'd be too > bug-prone to insist on leakproofness. So it seems worth the cost > to make the AM itself set up a throwaway memory context. > > (I don't recall offhand about which rule the other AMs use. > I'm also not sure where or if this choice is documented.) > > In the case at hand, I think the pfree is useless and was installed > by somebody who had mis-extrapolated from btree rules. So what > I'm thinking would be sufficient is to drop it altogether: > > - if (in != DatumGetArrayTypeP(entry->key)) > - pfree(in);
Looks like it's indeed a useless pfree call here - all paths that I could find that lead to GiST's compress procedure are encapsulated in a temporary context that is reset fairly quickly after use (at most this memory context would live for the duration of the recursive splitting of pages up the tree, but I haven't verified this hypotheses). There are similar pfree calls in the _int_gist.c file's g_int_compress function, which made me think we do need to clean up after use, but indeed these pfrees are useless (or even harmful if bug #17888 can be trusted) Kind regards, Matthias van de Meent.
