PostGIS provider cannot insert features with geometry

[cwilbur]

The code sample from FDO Developer’s Guide, Inserting Values, Page 92 fails with the PostGIS FDO provider.

The provider writes to the command line:

NOTICE: identifier "a400000000000003240000 00000008050400000000000003740000000000040524000000000000022400000000000004840000 00000000018400000000000004a400000000000003240050000000000000000804d4000000000000 032400000000000c0504000000000000032400000000000c050400000000000002a4000000000008 04d400000000000002a400000000000804d400000000000003240" will be truncated to "a40 000000000000324000000000008050400000000000003740000000000040" *** FDO Error: *** The execution of SQL statement failed with PostgreSQL error code: PGRES_FATAL_ERROR, ERROR: syntax error at or near "A40000000000000324000 00000000805040000000000000374000000000004052400000000000002240000000000000484000 000000000018400000000000004A400000000000003240050000000000000000804D400000000000 0032400000000000C0504000000000000032400000000000C050400000000000002A400000000000 804D400000000000002A400000000000804D400000000000003240" LINE 1: ...Blue Lake',010300000002000000050000000000000000004A400000000... ^

If the line in the sample that adds the geometry property to the value collection is removed, the insert succeeds.

FdoInt32 valueCollectionIndex = values->Add(sampleGeometryPropertyValue);

[cwilbur]

Patch for insert command

[cwilbur]

I don't think this is the best possible fix, but it's the lowest-impact fix. I still think the underlying code that uses PQexec should be changed to use PQexecParams and real parameterization, but by someone with more familiarity with the whole codebase of this provider.

[traianstanev]

Is it not better to implement the fix in ExpressionProcessor::ProcessGeometryValue()?

This way there would be no need to change the InsertCommand with a special case for geometry values.

[cwilbur]

Thanks, Traian, I didn't even look into the guts of the expression processor. I just saw it wasn't returning what I wanted. Added a new patch that throws out a lot of WKB-wrangling code and replaces it with calling ToString on the geometry.

[traianstanev]

This looks good, but if you could add a log statement the prints the geometry back it would be nice. Personally it doesn't matter to me, but seems like Mateusz consistently logs those things.

I expect fixing it in ExpressionProcessor would also fix a similar problem that would have manifested in the Update command, so it seems like an all around better fix now.

[cwilbur]

Fixed in expression processor with log message

[cwilbur]

Restored one of the two log messages and checked the log file for output after running tests

[traianstanev]

Thanks. Now who will check that in? :)

[jbirch]

I think I'd like some feedback on whether using WKB will limit the geometries (especially 3D and in future curve types) the provider can handle? Do you have any idea Chuck? I know that Mateusz and Paul Ramsey decided on the ewkb hex format for a reason...

[jbirch]

I've spoken with Paul about this a bit...

If you are using WKT with PostgreSQL, then it will limit you to two dimensions.

I'm not convinced that this is the correct approach to the problem. To me, this email

​http://www.nabble.com/Defects-in-PostGIS-provider-tf4313206s18162.html#a12282593

would indicate that the reason the insert is failing is because the hexwkb value is not being quoted. I believe that PostgreSQL is trying its best to detect the unquoted value as a number (the only thing that is allowed to be not quoted), and then interpreting the rest of the value as a column identifier (odd, but...).

Is there any way to add quotes to the built query? All values (even numbers) are allowed to be quoted in PostgreSQL.

[cwilbur]

This patch simply adds tick marks around the wkb string - it works as well as the GeomFromText fix in my tests

[mloskot]

I'm going to review and test these patches but I'm a bit confused. Are all these 3 patches to be applied or only two patches starting with ExprProc* should be considered?

There are some reasons why EWKB is used and transfered as HEX encoded stream. First, we intentionally do not use binary cursor so we do not exchange raw binary data. It would be even better to use Base64.

EWKB is used because it supports extended dimensions of coordinates used in FDO, but WKB does not support it.

Also, generally, it's recommended to work in text mode than in binary, because in the latter case we would need to deal with endianness issue. The PostgreSQL server accepts only big-endian binary, regardless of arch of machine the server is running on. This issue is not handled automatically by client library libpq.

Current strategy was discussed with Paul and Jason and the implementation follows the compromise we found.

[jbirch]

Mateusz, I believe that only Chuck's last patch should be considered, unless it fails some of your testing. I don't want to lose the ability to enter 3D data.

[cwilbur]

Jason, Mateusz, that's correct - the last patch is the latest suggested fix.

I'm not sure why you guys are so worried about losing the ability to enter 3D data when it looks to me like you don't have it now (from ApplySchemaCommand::AddGeometryColumn):

    // PostGIS Manual:
    // Strictly compliant OGC geometries cannot have Z or M values.
    // The IsValid() function won't consider higher dimensioned geometries invalid!
    // Invocations of AddGeometryColumn() will add a constraint checking geometry dimensions,
    // so it is enough to specify 2 there.
    int const dimension = 2;

I was going to run some tests with 3D data using the second (GeomFromText) and third (single quotes around WKB) proposed fixes, but the provider currently rejects 3D data completely. I'm guessing that's because it creates all geometry columns with 2 dimensions. InsertCommand::Execute aborts with an "Unsupported geometry type" exception if I pass 3D geometry.

[cwilbur]

Replying to cwilbur:

Jason, Mateusz, that's correct - the last patch is the latest suggested fix.

I'm not sure why you guys are so worried about losing the ability to enter 3D data when it looks to me like you don't have it now (from ApplySchemaCommand::AddGeometryColumn):

    // PostGIS Manual:
    // Strictly compliant OGC geometries cannot have Z or M values.
    // The IsValid() function won't consider higher dimensioned geometries invalid!
    // Invocations of AddGeometryColumn() will add a constraint checking geometry dimensions,
    // so it is enough to specify 2 there.
    int const dimension = 2;

I was going to run some tests with 3D data using the second (GeomFromText) and third (single quotes around WKB) proposed fixes, but the provider currently rejects 3D data completely. I'm guessing that's because it creates all geometry columns with 2 dimensions. InsertCommand::Execute aborts with an "Unsupported geometry type" exception if I pass 3D geometry.

P.S. I tried changing int const dimension to 3 and inserting 3D geometry. Both the "GeomFromText" and "quoted WKB" methods (patches 2 & 3, respectively) failed with an "invalid geometry" exception. I'm not sure what's going wrong here.

[mloskot]

I applied Chuck's patch ExprProcTicks.patch to the SVN (r3239).

I added inserting sample Point to ​insert_command sample (hosted outside the FDO SVN).

[mloskot]

Chuck, you're right about incomplete handling of 3 and 4 dims. I applied some fixes realated to this issue (r3240, see also ticket #108) but there is still problem in ExpressionProcessor class, where geometry is dumped to WKB using FdoFgfGeometryFactory::GetWKB() insead to EWKB with custom writer.

My suggestion is to still keep moving toward 3 and 4 dim support and using EWKB internally. I will continue my work and add EWKB writing support.

[mloskot]

I think we can close this ticket. The 3-4 dim support is covered by #108.