Re: BUG #15378: SP-GIST memory context screwup?

On Tue, Sep 11, 2018 at 6:13 AM Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>
wrote:

> [CCing Teodor as committer of ccd6eb49a]
>
> >>>>> "PG" == PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
>
> PG> I found this while analyzing a report from IRC that initially
> PG> looked like a PostGIS bug, but which I now think is breakage in
> PG> spgist:
>
> PG> spgrescan starts out by doing
> PG> MemoryContextReset(so->traversalCxt);
>
> PG> then later it calls resetSpGistScanOpaque(so);
> PG> which calls freeScanStack(so)
> PG> which calls freeScanStackEntry(so)
> PG> which does:
>
> PG> if (stackEntry->traversalValue)
> PG> pfree(stackEntry->traversalValue);
>
> PG> But stackEntry->traversalValue, if not NULL, is supposed to have
> PG> been allocated in so->traversalCxt, and so it's already gone.
> [...]
> PG> Unfortunately I don't think this can be demonstrated with the
> PG> built-in spgist opclasses, which don't allocate traversalValues.
>
> Turns out I was looking in the wrong place, and in fact we can reproduce
> this easily (at least on an assert build):
>
> create table boxes (b box);
> insert into boxes
> select box(point(i,j),point(i+s,j+s))
> from generate_series(1,100,5) i,
> generate_series(1,100,5) j,
> generate_series(1,10) s;
> create index on boxes using spgist (b);
> select *
> from (values
> (box(point(5,5),point(8,8))),(box(point(2,2),point(12,12)))) v(b)
> cross join lateral (select * from boxes where boxes.b && v.b limit
> 1) pt;
>
> So this logic was added in ccd6eb49a and was wrong from the start.
> Testing suggests that removing the offending pfree does indeed fix the
> issue; any objections?
>

No objections from me. What about
moving MemoryContextReset(so->traversalCxt) into resetSpGistScanOpaque()?
For me it seems that resetting of traversal memory context is part of
opaque reset.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company