#2293 closed defect (fixed)
crash on ST_DumpPoints with curved geoms
Reported by: | robe | Owned by: | nw |
---|---|---|---|
Priority: | blocker | Milestone: | PostGIS 2.1.0 |
Component: | postgis | Version: | master |
Keywords: | Cc: | nw@… |
Description
I started running thru my garden tests. I'll double check my old test. But I started testing using r10363 on 9.2 edb win 32bit and that went thru without crashing.
Then I upgraded to latest:
POSTGIS="2.1.0SVN r11330" GEOS="3.4.0dev-CAPI-1.8.0 r0" PROJ="Rel. 4.8.0, 6 March 2012" GDAL="GDAL 1.10.0, released 2013/04/24 GDAL_DATA not found" LIBXML="2.7.8" LIBJSON="UNKNOWN" RASTER PostgreSQL 9.2.0, compiled by Visual C++ build 1600, 32-bit
Crashola on this query:
SELECT ST_DumpPoints(foo1.the_geom) As result FROM ((SELECT ST_GeomFromEWKT('SRID=4326;CURVEPOLYGON(CIRCULARSTRING(-71.0821 42.3036, -71.4821 42.3036, -71.7821 42.7036, -71.0821 42.7036, -71.0821 42.3036),(-71.1821 42.4036, -71.3821 42.6036, -71.3821 42.4036, -71.1821 42.4036) ) ') As the_geom)) As foo1 LIMIT 3;
I suspect this may be caused by recent improvements on curved support.
Attachments (1)
Change History (15)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
Confirmed crash with POSTGIS="2.1.0SVN r11321" on PostgreSQL 9.1.9 on x86_64-unknown-linux-gnu, compiled by gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3, 64-bit
follow-up: 9 comment:3 by , 11 years ago
hmm — you suppose this TODO error: is a sign from somebody?
http://postgis.net/docs/doxygen/2.1/dc/d01/lwgeom__dumppoints_8c_source.html#00201
case CURVEPOLYTYPE: default: /* TODO error? */ if (--state->stacklen == 0) SRF_RETURN_DONE(funcctx); state->pathlen--; continue;
comment:4 by , 11 years ago
oops screwed up the link http://postgis.net/docs/doxygen/2.1/dc/d01/lwgeom__dumppoints_8c_source.html#l00201
comment:5 by , 11 years ago
it sounds like there should be a break in there, substituting the three lines under the TODO. Not that the break should affect the crash (culprit doesn't seem to be there). Valgrind would help here.
comment:6 by , 11 years ago
Cc: | added |
---|
Nathan,
Can you look at this one and provide a patch if you can. we'd like to have this resolved before we call our first 2.1 beta in prep for 9.3 beta1.
comment:7 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Will do. IIRC, the original function essentially ignored curved types. What exactly *should* be the behavior here? Dump what I assume are the control points? Return NULL? Return an empty array? Throw an exception? Interpolate points along the curve?
comment:8 by , 11 years ago
I think you should dump the points (which I believe are 3 points-on-curve per segment). But the crash suggests there's a memory error somewhere, as it should still be possible to ignore unsupported geometries without crashing. So I think you should first make it stable w/out supporting curves and only later, if you want, add support for the curve.
comment:9 by , 11 years ago
Replying to robe:
hmm — you suppose this TODO error: is a sign from somebody?
Actually, that was me wondering if st_dumppoints() should throw an error rather than just ignoring geometries it didn't understand.
by , 11 years ago
Attachment: | dumppoints.patch added |
---|
comment:11 by , 11 years ago
I have attached a patch made with 'git diff' that adds two tests on curved geometry to the dumppoints regression test, and changes the behavior to throw a data exception in case of unknown or unhandled geometries. It passes a make check on my system, which is postgresql 9.2.4, on a 64 bit AMD linux system. I don't think there's anything about the code that would cause it to behave differently in a different environment.
I would be happy to have it dump out the control points for curved geoms, but (per suggestion) let's open a different ticket for that.
Given that I tried to duplicate exactly the behavior of the plpgsql dumppoints function, I think that throwing an error here may be a change in behavior. In any case, the behavior should be documented. I can submit a doc patch if needed and desired, though I will need to install some more software to build the docs.
comment:12 by , 11 years ago
That's fine (I can add that to disclaimer of docs). I thought the old ST_DumpPoints did dump curved geoms so that sounds like a breaking change. I could be wrong though. I'll test on my old 2.0 and if that is the case, we'd probably want to get it in 2.1 (since it doesn't change the API and is a breaking change from 2.0), it would be marked as a bug fix rather than an enhancement so doesn't have to be done before our 2.1 beta release.
comment:13 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
committed at r11346. I'll update the docs next and create a separate ticket to support curved geoms.
comment:14 by , 11 years ago
Okay I was mistaken — i guess 2.0 did not support this either though throws this error:
ERROR: Unexpected error while dumping geometry CIRCULARSTRING(-71.0821 42.3036,-71.4821 42.3036,-71.7821 42.7036,-71.0821 42.7036,-71.0821 42.3036) CONTEXT: PL/pgSQL function _st_dumppoints(geometry,integer[]) line 22 at FOR over SELECT rows SQL function "st_dumppoints" statement 1
I'll add curved support as a separate ticket.
actually might be caused by the making of ST_DumpPoints a native C function. I realized the pre-version I was testing with r10363 (that did not crash) was about a month or so before ST_DumpPoints was flipped to a C function.