Opened 6 years ago

Closed 5 years ago

#4216 closed defect (fixed)

Performance degradation in method `contains`

Reported by: alvherre Owned by: pramsey
Priority: medium Milestone: PostGIS 2.4.6
Component: postgis Version: 2.4.x
Keywords: Cc: pramsey@…

Description

Github-mirror commit 8b21fa7d56ea851628a80cf2dd29ca565344c758 (SVN 15778) caused a severe performance degradation when the ~ operator is applied to the result of a BitmapIndexScan. The reason is that full detoasting is now applied to every tuple coming out of the index, as opposed to only the first BOX2DF-sized slice as the previous code did.

This doesn't affect tuples coming out of heap (table), because as the new comment correctly states, the whole datum has to be fetched anyway; but apparently indexes don't support compressed toast so the slice mechanism worked just fine for those.

This causes a serious performance degradation of a real-world use case — query goes from 188ms to 3658ms.

I don't yet have a patch or a readily publishable reproducer to share, but I can look into it if necessary.

Change History (9)

comment:1 by komzpa, 6 years ago

Can you share more details on the query? GiST index doesn't even store geometry and just keeps the box.

How did you find the offending commit? Can it happen that there is another reason?

What storage does your heap use?

comment:2 by alvherre, 6 years ago

Hi, thanks for your quick response.

Can you share more details on the query?

Sadly, I cannot share the original query. I'll try to create a reproducer, but I don't have sample GIS data yet, so it'll take some time. I'll send more details on the shape of the query and plan in my next comment, to be written immediately after sending this one. The table is about 700 GB.

GiST index doesn't even store geometry and just keeps the box.

Oh, that makes sense in retrospect.

How did you find the offending commit? Can it happen that there is another reason?

Well, we can see that there's a much higher buffer hit count in EXPLAIN ANALYZE and no explanation for that in the Postgres code; looking (as a newcomer to PostGIS) at the code implementing the ~ operator it seems that this is the only possibility.

This is an application running on 2.2; degradation was found upgrading to 2.4; downgrading to 2.3 restores performance. I have not yet tried to measure performance with only that commit reverted, but I'll do that too when I have a reproducer.

comment:3 by komzpa, 6 years ago

Presence of BitmapIndexScan in PostGIS queries is also usually a sign of work_mem being set too low.

Please also figure out what storage your table uses, main or extended. If it's extended it's not supposed to be affected by this change. If it's main, interesting who and why changed it from default :)

Also @pramsey had a prototype of partial detoasting of compressed values. If it ever goes to postgres maybe it makes sense to revert the change? :)

As a temporary solution, ND index still has the detoast-header code, so you can try to rebuild with ND opclass.

in reply to:  3 comment:4 by alvherre, 6 years ago

Replying to komzpa:

Presence of BitmapIndexScan in PostGIS queries is also usually a sign of work_mem being set too low.

This is not at all the case here:

               Sort Method: quicksort  Memory: 25kB

The reason there's a BitmapIndexScan here is because there are two indexes involved (one is a Btree; the other is the geometry column in question), and they're being BitmapAnd'ed.

Please also figure out what storage your table uses, main or extended. If it's extended it's not supposed to be affected by this change. If it's main, interesting who and why changed it from default :)

Storage is per column, not per table, and it's "main" for all the geometry columns… which matches what Ramsey mentioned in the code comment he added in the commit I cited.

As a temporary solution, ND index still has the detoast-header code, so you can try to rebuild with ND opclass.

Sounds like it'd be slower than the 2D 2.3 code, in which case it's not a great solution going forward.

comment:5 by pramsey, 6 years ago

I think I'll just revert, as the change didn't have any positive effect either…

comment:6 by pramsey, 6 years ago

In 16969:

Revert change to avoid slicing on box access.
References #4216

comment:7 by pramsey, 6 years ago

Please test the head of 2.4 branch to see if the reversion fixes the performance issue.

comment:8 by pramsey, 5 years ago

In 16995:

Revert change to avoid slicing on box access.
References #4216

comment:9 by pramsey, 5 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.