[REVIEW] PostGIS provider patch 3.4 from Bruno Scott

[mloskot]

This task is about reviewing Bruno Scott's big patch for the PostGIS provider posted to the list - ​PATCH : PostGIS provider patch 3.4

Here is list of tickets fixed by this patch:

#94 Generate extent for features assigned to default spatial context

#106 PostGIS provider cannot delete a feature class

#171 Fdo Postgis Autogenated identity property is mandatory

#178 PostGIS : Can't insert in a non-feature class

#232 Fdo Postgis null and not null filter does not work

#233 Fdo Postgis in and not in filter does not work

#234 Fdo Postgis currently does not support anything but lowercase identifiers

#235 Fdo Postgis Exception with insert

#236 Fdo Postgis does not support non spatial classes

#241 Implement Support for SelectAggregates, SpatialExtents and Count

#310 PostGIS provider : change class naming convention without ~

#311 PostGIS provider : mismatch between FdoGeometricTypes and FdoGeometryType

#312 PostGIS provider : remove boost_date_time dll dependency

#313 PostGIS provider : can't filter,insert or update date/datetime

#314 PostGIS provider : check-out/check-in crashes Autodesk Map

mloskot: Big thanks to Bruno for this patch!

[mloskot]

PostGIS provider big patch from Bruno Scott

[mloskot]

In the patch for #310, there is one place unclear to me, where original version (line 228) is:

-        FdoStringP fdoClassName = FdoStringP::Format(L"%s~%s",
-            static_cast<FdoString*>(stReader->GetSchemaName()),
-            static_cast<FdoString*>(stReader->GetTableName()));

and patched version (line 213) is:

+        //fdoClassName = FdoStringP::Format(L"%s~%s",
+        //    static_cast<FdoString*>(stReader->GetSchemaName()),
+        //    static_cast<FdoString*>(stReader->GetTableName() ));

In both versions the format is the same and uses old ~.

[mloskot]

Bruno, do you mind if I'd suggest to remove comments about ticket number, what was fixed, etc. after the patch is applied?

For example, here the first line seems redundant to me (after the patch is applied to the FDO tree):

/// ticket #310 - class naming convention wihout ~ - eric barby 
/// Set schema name (physical schame of postgres). 
FDOPOSTGIS_API void SetSchemaName(FdoString* value); 

[mloskot]

Patch for ticket #312 (remove boost_date_time dll dependency) looks good.

[mloskot]

AFAIU, the patch for ticket #314 (check-out/check-in crashes Autodesk Map) is a temporary fix. If that's right, I'd suggest to keep the #314 open.

[bscott]

According to the ticket 314 description "check-out/check-in crashes Autodesk Map" Map does not crash anymore so i think we should close that ticket and create a new ticket

"PostGIS provider does not support Transaction"

[mloskot]

Bruno,

I see you've added two new files:

Providers/PostGIS/Src/Provider/PgTablesReader.h
Providers/PostGIS/Src/Provider/PgTablesReader.cpp

Certainly, I'm completely fine with them. I just need to ask you to fix the copyright notice by replacing the 2nd line:

// Copyright (C) 2007 Refractions Research, Inc. 

to read

// Copyright (C) 2008 ...

where ... is name of the author of this file or company.

Please, tell me what to use as (C) and I'll apply the fix.

[mloskot]

Bruno,

Regardless of my question in the 3rd comment above, I'm removing these 3 lines which are commented and use old formatting string with tilde.

[bscott]

Actually these two files are not really new one. Providers/PostGIS/Src/Provider/PgTablesReader.h Providers/PostGIS/Src/Provider/PgTablesReader.cpp They are just PgSpatialTablesReader.* renamed to PsTablesReader.* as they not only read spatial table but also non spatial tables. It makes thing clearer :)

[mloskot]

Bruno,

Ah, got it. I've mistaken it because the patch does not include delete action. OK, clear.

[mloskot]

Bruno,

Is this intentional that in file Providers/PostGIS/Src/Provider/DeleteCommand.cpp, lines 49-51:

// eric barby - faster way of describing schema 
SchemaDescription *schemaDesc = mConn->DescribeSchema(); 
if (!schemaDesc || !schemaDesc->IsDescribed())  

schemaDesc is not wrapped with FdoPtr ?

The Connection::DescribeSchema increments reference counter to the schema description object. So, client of the schemaDesc is supposed to decrement it, ideally using automatic collection with FdoPtr.

[mloskot]

Just FYI, it's recommended to not to use vendor-specific API, but to use portable replacements like: prefer FdoCommonOSUtil::wcsicmp instead of _wcsicmp.

[mloskot]

In file Providers/PostGIS/Src/Provider/InsertCommand.cpp, line 195, there is following comment:

// eric barby - need to parse date properly 

Is this a kind of TODO saying that the code below should be improved in future or it's just a description of the code below as the actual improvement?

[mloskot]

Bruno,

I'd have a suggestion to mark lines that require some future review/fixes/improvements/etc. with well-known markers like:

XXX
TODO
FIXME
NOTE

Here, I'm referring to your comment in Providers/PostGIS/Src/Provider/InsertCommand.cpp, line 204:

// bruno scott - geom is not used anywhere, maybe we should delete this line 

Markers simply help to grep through the sources and look for stuff that needs some work. Also, marked lines are displayed in the Tasks List window in Visual Studio :-)

[mloskot]

Hmm, I've encountered a few replacements like this:

- std::string sqlSelect; 
+ std::string sqlSelect("");

Actually, these lines have the same effect, but I'm just curious: Have you experienced problems with improperly default constructed std::string so you decided to explicitly initialize it? If yes, what compiler?

[mloskot]

Replying to mloskot:

Bruno,

Is this intentional that in file Providers/PostGIS/Src/Provider/DeleteCommand.cpp, lines 49-51:

Actually this question applies to {Insert|Update}Command.cpp files.

[mloskot]

IMPORTANT

The patch has been applied to the trunk (r3904).

Big thanks to Bruno, Eric and their Team for the fantastic work. I believe I can close this ticket. Please, reopen if needed.

[mloskot]

Bruno,

Sorry for bothering, could you review lines 49-51 in file Providers/PostGIS/Src/Provider/DeleteCommand.cpp why the schema description is not grabbed with FdoPtr?