Re: BUG #15378: SP-GIST memory context screwup? - Mailing list pgsql-bugs

From Alexander Korotkov
Subject Re: BUG #15378: SP-GIST memory context screwup?
Date
Msg-id CAPpHfdvbVMjB7ORQeXRxMX65u7P-TfG66bZh5U7+3j2QTAov-A@mail.gmail.com
Whole thread Raw
In response to Re: BUG #15378: SP-GIST memory context screwup?  (Andrew Gierth <andrew@tao11.riddles.org.uk>)
Responses Re: BUG #15378: SP-GIST memory context screwup?
Re: BUG #15378: SP-GIST memory context screwup?
List pgsql-bugs
On Tue, Sep 11, 2018 at 6:13 AM Andrew Gierth <andrew@tao11.riddles.org.uk> wrote:
[CCing Teodor as committer of ccd6eb49a]

>>>>> "PG" == PG Bug reporting form <noreply@postgresql.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 

pgsql-bugs by date:

Previous
From: Tom Lane
Date:
Subject: Re: `pg_trgm` not recognizing Chinese characters in macOS
Next
From: Andrew Gierth
Date:
Subject: Re: BUG #15378: SP-GIST memory context screwup?