Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GiST test case for #4139 #336

Closed
wants to merge 14 commits into from

Conversation

@Komzpa
Copy link
Member

commented Nov 20, 2018

See previous discussion in #301

References https://trac.osgeo.org/postgis/ticket/4139

@Komzpa Komzpa referenced this pull request Nov 20, 2018
@Komzpa

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

It feels like argument order around
https://github.com/postgis/postgis/blob/svn-trunk/postgis/gserialized_gist_nd.c#L1064
is swapped. @alesuiss could that be the case, that during tree traversal containedby and contains are swapped, and random nature of the gist tree brings gets randomized results?

Komzpa added 7 commits Nov 21, 2018
@Komzpa

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

@Algunenano the problem was indeed during index construction. gidx_merge was adding more constraining dimensions instead of dropping unshared.

Tests pass their counts, but I get memory warnings right in Postgres output that I don't yet know how to even approach:

--- regress_gist_index_nd_expected      2018-11-21 11:04:05.293839600 +0300
+++ /tmp/pgis_reg/test_66_out   2018-11-22 01:06:45.174052881 +0300
@@ -1,3 +1,10 @@
+WARNING:  detected write past chunk end in GiST temporary context 0x5635287b01a0
+WARNING:  detected write past chunk end in GiST temporary context 0x5635287b0168
+WARNING:  detected write past chunk end in GiST temporary context 0x5635287afcc8
+WARNING:  detected write past chunk end in GiST temporary context 0x5635287b0108
+WARNING:  detected write past chunk end in GiST temporary context 0x5635287afc68
+WARNING:  problem in alloc set GiST temporary context: bad single-chunk 0x5635287b0140 in block 0x5635287ad570
+WARNING:  problem in alloc set GiST temporary context: found inconsistent memory block 0x5635287ad570
 &&&|180502|Seq Scan|180502|Index Scan
 ~~ |39682|Seq Scan|39682|Index Scan
 @@ |39682|Seq Scan|39682|Index Scan
/tmp/pgis_reg/test_66_diff (END)
Komzpa added 2 commits Nov 21, 2018
Remove now-invalid memcpy optimization
Thanks RhodiumToad on IRC
we need to initialize the first value
memcpy -> pfree; gidx_copy

Thanks Andrew Gierth aka RhodiumToad on IRC for noticing
@@ -180,15 +179,16 @@ gidx_merge(GIDX **b_union, GIDX *b_new)

POSTGIS_DEBUGF(4, "merging gidx (%s) into gidx (%s)", gidx_to_string(b_new), gidx_to_string(*b_union));

if (dims_new > dims_union)
/* Shrink unshared dimensions */
if (dims_new < dims_union)

This comment has been minimized.

Copy link
@Algunenano

Algunenano Nov 21, 2018

Member

Is this shrinking really necessary now? If b_union has 10 dimensions and b_new 2, I'd expect the merge to have 10, not 2; that is, a merge between 2 boxes should be bigger, never smaller.

This comment has been minimized.

Copy link
@Komzpa

Komzpa Nov 22, 2018

Author Member

Think of 3DM box: it has (minx, maxx, miny, maxy, -FLT_MAX, FLT_MAX, minm, maxm ) for padding, it is a 4D gidx.

When this gets into calculations here, it resets whatever was there in Z to -flt_max / flt_max, in gidx_merge logic. That is equivalent of dropping the dimension. Thus, unshared dimensions should be dropped to correctly summarize a group of other mixed-dimensions boxes.

Is there a fault in this logic?

This comment has been minimized.

Copy link
@Algunenano

Algunenano Nov 22, 2018

Member

Think of 3DM box: it has (minx, maxx, miny, maxy, -FLT_MAX, FLT_MAX, minm, maxm ) for padding, it is a 4D gidx.

That's what I wasn't expecting. I'd expect that a XYM GBOX would result in a 3-dimension gidx, but as you say it's done by adding an infinite dimension instead [1]. Based on the preferences taken ([2]), I'm unsure whether gidx_merge the union should contain that extra dimension or not, but it appears that this was in fact the issue here.

[1]

/* if M is present but Z is not, pad Z and shift M */

[2] https://lists.osgeo.org/pipermail/postgis-devel/2015-February/024759.html

@Algunenano

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

This patch is fixing the memory issue described in: https://trac.osgeo.org/postgis/ticket/4237

Before I was getting warnings this this when the index was being created:

 0x7f8625e3edb3 in gidx_merge /home/raul/dev/public/postgis/postgis/gserialized_gist_nd.c:194:3
 0x7f8625e65c53 in gserialized_gist_picksplit_addlist /home/raul/dev/public/postgis/postgis/gserialized_gist_nd.c:1427:3
 0x7f8625e605c9 in gserialized_gist_picksplit /home/raul/dev/public/postgis/postgis/gserialized_gist_nd.c:1712:5
 0x5599ca37d7d7 in FunctionCall2Coll /usr/src/debug/postgres/src/backend/utils/fmgr/fmgr.c:1145:11
 0x5599c8228e10 in gistUserPicksplit /usr/src/debug/postgres/src/backend/access/gist/gistsplit.c:433:2
 0x5599c8220d8c in gistSplitByKey /usr/src/debug/postgres/src/backend/access/gist/gistsplit.c:697:7
 0x5599c81d5d82 in gistSplit /usr/src/debug/postgres/src/backend/access/gist/gist.c:1382:2
 0x5599c81cfc99 in gistplacetopage /usr/src/debug/postgres/src/backend/access/gist/gist.c:296:10
 0x5599c81ce4ff in gistinserttuples /usr/src/debug/postgres/src/backend/access/gist/gist.c:1229:13
 0x5599c81ce4ff in gistinserttuple /usr/src/debug/postgres/src/backend/access/gist/gist.c:1182
 0x5599c81ce4ff in gistdoinsert /usr/src/debug/postgres/src/backend/access/gist/gist.c:839
 0x5599c8233990 in gistBuildCallback /usr/src/debug/postgres/src/backend/access/gist/gistbuild.c:486:3
 0x5599c85ec0d0 in IndexBuildHeapRangeScan /usr/src/debug/postgres/src/backend/catalog/index.c:2950:4
 0x5599c85e99e3 in IndexBuildHeapScan /usr/src/debug/postgres/src/backend/catalog/index.c:2458:9
 0x5599c823224e in gistbuild /usr/src/debug/postgres/src/backend/access/gist/gistbuild.c:205:14
 0x5599c85de67e in index_build /usr/src/debug/postgres/src/backend/catalog/index.c:2320:10
 0x5599c85da402 in index_create /usr/src/debug/postgres/src/backend/catalog/index.c:1219:3
 0x5599c8a9aba3 in DefineIndex /usr/src/debug/postgres/src/backend/commands/indexcmds.c:846:3
 0x5599c9a2a347 in ProcessUtilitySlow /usr/src/debug/postgres/src/backend/tcop/utility.c:1355:7
 0x5599c9a22eca in standard_ProcessUtility /usr/src/debug/postgres/src/backend/tcop/utility.c
 0x5599c9a2116b in ProcessUtility /usr/src/debug/postgres/src/backend/tcop/utility.c:360:3
 0x5599c9a1f293 in PortalRunUtility /usr/src/debug/postgres/src/backend/tcop/pquery.c:1178:2
 value was stored to memory at
 0x7f8625e3edb3 in gidx_merge /home/raul/dev/public/postgis/postgis/gserialized_gist_nd.c:194:3

With this patch applied I no longer see those warnings.

@strk strk closed this in 1cb0fb4 Nov 24, 2018

@Algunenano

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

Great job with this @Komzpa !

@Komzpa Komzpa deleted the ticket-4139 branch Nov 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.