Opened 6 years ago

Closed 5 years ago

#3979 closed patch (fixed)

[Re]introduce postgis_geos_noop() and postgis_sfcgal_noop()

Reported by: lucasvr Owned by: pramsey
Priority: medium Milestone: PostGIS 3.0.0
Component: postgis Version: 2.4.x
Keywords: serialization deserialization performance Cc:

Description

PostGIS used to expose a SQL function named geosnoop(geometry) to test the cost of deserializing and reserializing from the PostgreSQL backend. The attached patch brings that function back (this time named as postgis_geos_noop(geometry)) together with its SFCGAL counterpart (postgis_sfcgal_noop(geometry)).

Attachments (2)

postgis-noop.patch (2.7 KB ) - added by lucasvr 6 years ago.
postgis-noop-v2.patch (4.1 KB ) - added by lucasvr 6 years ago.
Revamped version of the original patch

Download all attachments as: .zip

Change History (10)

by lucasvr, 6 years ago

Attachment: postgis-noop.patch added

comment:1 by strk, 6 years ago

Can you please also add tests under regress/ directory ? What i get is:

strk=# select st_npoints(postgis_sfcgal_noop(ST_Buffer('POINT(0 0)', 5)));
ERROR:  Unknown geometry type: 8208 - Invalid type

Other function (geos) is fine.

For tests I see also postgis_noop testing is missing, you could add that one in regress/regress.sql togheter with the postgis_geos_noop, and the geos_sfcgal_noop would go in regress_sfcgal.sql

comment:2 by strk, 6 years ago

for the bug itself it sounds like due to the fact you're not re-serializing the output (geometry_serialize)

comment:3 by robe, 6 years ago

Milestone: PostGIS 2.4.3PostGIS 2.5.0

Has to go in 2.5 since it adds to SQL API.

comment:4 by lucasvr, 6 years ago

It looks like I've attached the first diff that I produced for review purposes. Sorry about that, it was indeed missing the re-serializing part!

I have submitted a new patch (v2) that includes the re-serializing and the regression tests. I've also updated the comments to reflect that this API is available from 2.5.0 onwards. I appreciate your review and guidance on missing bits (if any).

Thanks!

Last edited 6 years ago by lucasvr (previous) (diff)

by lucasvr, 6 years ago

Attachment: postgis-noop-v2.patch added

Revamped version of the original patch

comment:5 by strk, 6 years ago

Sounds good. Only thing is (beside a missing TAB on line 792 of postgis/lwgeom_sfcgal.c) is I'd avoid the Buffer call in the test and give the geos_noop test a good label like the sfcgal one.

If you feel like doing more, ideally those noop tests would test not a single geometry but a range of geometry types and dimensions, ensuring for example that the SRID is retained upon round-trip and the Z value (GEOS does not support the M value, not sure about sfcgal, if it does it's worth testing that too) and the different types (not all types are supported by GEOS)

comment:6 by komzpa, 6 years ago

Milestone: PostGIS 2.5.0PostGIS 3.0.0

comment:7 by strk, 6 years ago

postgis_geos_noop was added, see #2902, so this ticket remains for postgis_sfcgal_noop. Are you willing to complete the patch to include in 3.0.0 lucasvr ?

comment:8 by komzpa, 5 years ago

Resolution: fixed
Status: newclosed

In 17727:

postgis_sfcgal_noop().

Patch by Lucas C. Villa Real <lucasvr@…>, tweaks by Darafei Praliaskouski

Closes #3979

Note: See TracTickets for help on using tickets.