RFC 68 for review

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC 68 for review

Bruno Scott
Hi all,

RFC 68 (http://trac.osgeo.org/fdo/wiki/FDORfc68) has been posted. It proposes to add case insensitive support to the PostgreSQL provider


Bruno Scott
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Greg Boone
In general I have no issues with the goals of the RFC.

Some initial feedback on the patch. I will do a deeper review and get back to you.


(a) What is the difference between the following 2 functions? They seem duplicates of each other. SupportsAnsiQuotes() is never used.

Index: Providers/GenericRdbms/Src/PostGis/SchemaMgr/Ph/Mgr.cpp
===================================================================
--- Providers/GenericRdbms/Src/PostGis/SchemaMgr/Ph/Mgr.cpp (revision 6964)
+++ Providers/GenericRdbms/Src/PostGis/SchemaMgr/Ph/Mgr.cpp (working copy)

bool FdoSmPhPostGisMgr::SupportsAnsiQuotes()
{
  if(mIsCaseSensitive)
  {
    return true;
  }
  else
  {
    return false;
  }
}

bool FdoSmPhPostGisMgr::IsCaseSensitive()
{
  return mIsCaseSensitive;
}



(b) Why did you remove the use of GIST_GEOMETRY_OPS below?

Index: Providers/GenericRdbms/Src/PostGis/SchemaMgr/Ph/SpatialIndex.cpp
===================================================================
--- Providers/GenericRdbms/Src/PostGis/SchemaMgr/Ph/SpatialIndex.cpp (revision 6964)
+++ Providers/GenericRdbms/Src/PostGis/SchemaMgr/Ph/SpatialIndex.cpp (working copy)
@@ -158,7 +158,7 @@
                 indexName = indexName.Right(L"."); // no schema
 
             sqlStmt = FdoStringP::Format(
-                L"CREATE INDEX \"%ls\"  ON %ls USING GIST (\"%ls\" GIST_GEOMETRY_OPS)",
+                L"CREATE INDEX \"%ls\"  ON %ls USING GIST (\"%ls\")",
                 (FdoString*) indexName,
                 (FdoString*) tableDbQName, // no dbo. prefix etc.
                 column->GetName()


(c) Can you update your indent/tab to be 4 spaces? That is the expected standard. Also. all tab characters should be replaced by 4 spaces.


(d) I believe you have a memory leak. New and no subsequent delete.

Index: Providers/GenericRdbms/Src/PostGis/Fdo/FdoRdbmsPostGisConnectionInfo.cpp
===================================================================
--- Providers/GenericRdbms/Src/PostGis/Fdo/FdoRdbmsPostGisConnectionInfo.cpp (revision 6964)
+++ Providers/GenericRdbms/Src/PostGis/Fdo/FdoRdbmsPostGisConnectionInfo.cpp (working copy)
@@ -92,6 +92,16 @@
                 NlsMsgGet(FDORDBMS_117, "DataStore"), L"",
                 false, false, true, false, false, true, false, 0, NULL);
         mPropertyDictionary->AddProperty(pProp);
+
+        wchar_t **boolValues = new wchar_t*[2];
+        boolValues[0] = new wchar_t[5];
+        boolValues[1] = new wchar_t[6];
+        wcscpy(boolValues[0], L"TRUE");
+        wcscpy(boolValues[1], L"FALSE");
+        pProp = new ConnectionProperty (FDO_RDBMS_CONNECTION_CASESENSITIVE, L"CaseSensitive", L"TRUE", false, false, true, false, false, false, false, 2, (const wchar_t**)boolValues);
+
+
+        mPropertyDictionary->AddProperty(pProp);
     }



Regards,
Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Bruno Scott
Sent: Wednesday, September 11, 2013 8:54 AM
To: [hidden email]
Subject: [fdo-internals] RFC 68 for review

Hi all,

RFC 68 (http://trac.osgeo.org/fdo/wiki/FDORfc68) has been posted. It proposes to add case insensitive support to the PostgreSQL provider


Bruno Scott



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/RFC-68-for-review-tp5077290.html
Sent from the FDO Internals mailing list archive at Nabble.com.
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Bruno Scott
In reply to this post by Bruno Scott
Hi Greg

(a) What is the difference between the following 2 functions? They seem duplicates of each other. SupportsAnsiQuotes() is never used.

the SupportsAnsiQuotes function is used in all providers derived from the genericrdbms(sqlserver,odbc...)
and it tells what it does. In the specific case of PostGis, PostGIS actually does support Ansi Quotes But is is the easiest way for having the generic part of the provider to remove the quotes. All the code i've added use the IsCaseSensitive function, i think it's clearer this way.


(b) Why did you remove the use of GIST_GEOMETRY_OPS below?

This is not related to the CaseSensitive patch. The GIST_GEOMETRY_OPS does not exist anymore in version 2.*. Only the 2.* runs on 64 bit server. The GIST_GEOMETRY_OPS key word makes the Fdo Create index failed. Removing it make it to work with the default values.

(c) Can you update your indent/tab to be 4 spaces? That is the expected standard. Also. all tab characters should be replaced by 4 spaces.

oops, i will post another patch tomorrow

(d) I believe you have a memory leak. New and no subsequent delete.

I will check this


Thanks for your comments Greg

Bruno
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Greg Boone
Re: (b)

The provider still needs to support 1.x. I thought we added if/else conditions or "fail-over" conditions to accommodate this need. For example,
- Execute the command using GIST_GEOMETRY_OPS.
- If the create index command fails using GIST_GEOMETRY_OPS, re-execute without this parameter.
- If the error persists, error out.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Bruno Scott
Sent: Wednesday, September 11, 2013 11:45 AM
To: [hidden email]
Subject: Re: [fdo-internals] RFC 68 for review

Hi Greg

(a) What is the difference between the following 2 functions? They seem duplicates of each other. SupportsAnsiQuotes() is never used.

the SupportsAnsiQuotes function is used in all providers derived from the
genericrdbms(sqlserver,odbc...)
and it tells what it does. In the specific case of PostGis, PostGIS actually does support Ansi Quotes But is is the easiest way for having the generic part of the provider to remove the quotes. All the code i've added use the IsCaseSensitive function, i think it's clearer this way.


(b) Why did you remove the use of GIST_GEOMETRY_OPS below?

This is not related to the CaseSensitive patch. The GIST_GEOMETRY_OPS does not exist anymore in version 2.*. Only the 2.* runs on 64 bit server. The GIST_GEOMETRY_OPS key word makes the Fdo Create index failed. Removing it make it to work with the default values.

(c) Can you update your indent/tab to be 4 spaces? That is the expected standard. Also. all tab characters should be replaced by 4 spaces.

oops, i will post another patch tomorrow

(d) I believe you have a memory leak. New and no subsequent delete.

I will check this


Thanks for your comments Greg

Bruno



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/RFC-68-for-review-tp5077290p5077327.html
Sent from the FDO Internals mailing list archive at Nabble.com.
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Greg Boone
Actually, the logic can be reversed, since 2.0 is the new standard.

- Execute the command without using GIST_GEOMETRY_OPS.
- If the command fails, re-execute using GIST_GEOMETRY_OPS.
- If the error persists, error out.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Greg Boone
Sent: Wednesday, September 11, 2013 11:54 AM
To: FDO Internals Mail List
Subject: Re: [fdo-internals] RFC 68 for review

Re: (b)

The provider still needs to support 1.x. I thought we added if/else conditions or "fail-over" conditions to accommodate this need. For example,
- Execute the command using GIST_GEOMETRY_OPS.
- If the create index command fails using GIST_GEOMETRY_OPS, re-execute without this parameter.
- If the error persists, error out.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Bruno Scott
Sent: Wednesday, September 11, 2013 11:45 AM
To: [hidden email]
Subject: Re: [fdo-internals] RFC 68 for review

Hi Greg

(a) What is the difference between the following 2 functions? They seem duplicates of each other. SupportsAnsiQuotes() is never used.

the SupportsAnsiQuotes function is used in all providers derived from the
genericrdbms(sqlserver,odbc...)
and it tells what it does. In the specific case of PostGis, PostGIS actually does support Ansi Quotes But is is the easiest way for having the generic part of the provider to remove the quotes. All the code i've added use the IsCaseSensitive function, i think it's clearer this way.


(b) Why did you remove the use of GIST_GEOMETRY_OPS below?

This is not related to the CaseSensitive patch. The GIST_GEOMETRY_OPS does not exist anymore in version 2.*. Only the 2.* runs on 64 bit server. The GIST_GEOMETRY_OPS key word makes the Fdo Create index failed. Removing it make it to work with the default values.

(c) Can you update your indent/tab to be 4 spaces? That is the expected standard. Also. all tab characters should be replaced by 4 spaces.

oops, i will post another patch tomorrow

(d) I believe you have a memory leak. New and no subsequent delete.

I will check this


Thanks for your comments Greg

Bruno



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/RFC-68-for-review-tp5077290p5077327.html
Sent from the FDO Internals mailing list archive at Nabble.com.
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Greg Boone
Actually, I may have spoken too soon. I don't really know if that command will fail on 1.0 without GIST_GEOMETRY_OPS being set.

Needs to be tested.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Greg Boone
Sent: Wednesday, September 11, 2013 7:58 PM
To: FDO Internals Mail List
Subject: Re: [fdo-internals] RFC 68 for review

Actually, the logic can be reversed, since 2.0 is the new standard.

- Execute the command without using GIST_GEOMETRY_OPS.
- If the command fails, re-execute using GIST_GEOMETRY_OPS.
- If the error persists, error out.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Greg Boone
Sent: Wednesday, September 11, 2013 11:54 AM
To: FDO Internals Mail List
Subject: Re: [fdo-internals] RFC 68 for review

Re: (b)

The provider still needs to support 1.x. I thought we added if/else conditions or "fail-over" conditions to accommodate this need. For example,
- Execute the command using GIST_GEOMETRY_OPS.
- If the create index command fails using GIST_GEOMETRY_OPS, re-execute without this parameter.
- If the error persists, error out.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Bruno Scott
Sent: Wednesday, September 11, 2013 11:45 AM
To: [hidden email]
Subject: Re: [fdo-internals] RFC 68 for review

Hi Greg

(a) What is the difference between the following 2 functions? They seem duplicates of each other. SupportsAnsiQuotes() is never used.

the SupportsAnsiQuotes function is used in all providers derived from the
genericrdbms(sqlserver,odbc...)
and it tells what it does. In the specific case of PostGis, PostGIS actually does support Ansi Quotes But is is the easiest way for having the generic part of the provider to remove the quotes. All the code i've added use the IsCaseSensitive function, i think it's clearer this way.


(b) Why did you remove the use of GIST_GEOMETRY_OPS below?

This is not related to the CaseSensitive patch. The GIST_GEOMETRY_OPS does not exist anymore in version 2.*. Only the 2.* runs on 64 bit server. The GIST_GEOMETRY_OPS key word makes the Fdo Create index failed. Removing it make it to work with the default values.

(c) Can you update your indent/tab to be 4 spaces? That is the expected standard. Also. all tab characters should be replaced by 4 spaces.

oops, i will post another patch tomorrow

(d) I believe you have a memory leak. New and no subsequent delete.

I will check this


Thanks for your comments Greg

Bruno



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/RFC-68-for-review-tp5077290p5077327.html
Sent from the FDO Internals mailing list archive at Nabble.com.
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Andy Zhang
In reply to this post by Greg Boone
Hi Bruno,

It looks like you are NOT working on the latest version of Providers\GenericRdbms\Src\PostGis\SchemaMgr\Ph\SpatialIndex.cpp. I removed the GIST_GEOMETRY_OPS in revision #6796 when implementing ticket #764.

PostGis 1.5 works without this parameter. Below statement is from PostGis 2.0 manual.
"Some older applications that as part of the process create tables and indexes, explicitly referenced the operator class name. This was unnecessary if you want the default 2D index."
So we think it is safe to remove the parameter.

Thanks
Andy

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Greg Boone
Sent: Thursday, September 12, 2013 7:58 AM
To: FDO Internals Mail List
Subject: Re: [fdo-internals] RFC 68 for review

Actually, the logic can be reversed, since 2.0 is the new standard.

- Execute the command without using GIST_GEOMETRY_OPS.
- If the command fails, re-execute using GIST_GEOMETRY_OPS.
- If the error persists, error out.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Greg Boone
Sent: Wednesday, September 11, 2013 11:54 AM
To: FDO Internals Mail List
Subject: Re: [fdo-internals] RFC 68 for review

Re: (b)

The provider still needs to support 1.x. I thought we added if/else conditions or "fail-over" conditions to accommodate this need. For example,
- Execute the command using GIST_GEOMETRY_OPS.
- If the create index command fails using GIST_GEOMETRY_OPS, re-execute without this parameter.
- If the error persists, error out.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Bruno Scott
Sent: Wednesday, September 11, 2013 11:45 AM
To: [hidden email]
Subject: Re: [fdo-internals] RFC 68 for review

Hi Greg

(a) What is the difference between the following 2 functions? They seem duplicates of each other. SupportsAnsiQuotes() is never used.

the SupportsAnsiQuotes function is used in all providers derived from the
genericrdbms(sqlserver,odbc...)
and it tells what it does. In the specific case of PostGis, PostGIS actually does support Ansi Quotes But is is the easiest way for having the generic part of the provider to remove the quotes. All the code i've added use the IsCaseSensitive function, i think it's clearer this way.


(b) Why did you remove the use of GIST_GEOMETRY_OPS below?

This is not related to the CaseSensitive patch. The GIST_GEOMETRY_OPS does not exist anymore in version 2.*. Only the 2.* runs on 64 bit server. The GIST_GEOMETRY_OPS key word makes the Fdo Create index failed. Removing it make it to work with the default values.

(c) Can you update your indent/tab to be 4 spaces? That is the expected standard. Also. all tab characters should be replaced by 4 spaces.

oops, i will post another patch tomorrow

(d) I believe you have a memory leak. New and no subsequent delete.

I will check this


Thanks for your comments Greg

Bruno



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/RFC-68-for-review-tp5077290p5077327.html
Sent from the FDO Internals mailing list archive at Nabble.com.
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Bruno Scott
In reply to this post by Greg Boone
Hi Greg
Getting back on the memory leaks thing.
I think there is no memory leaks, there's lots of places where we found the same pieces of code.


This is the code,
        wchar_t **boolValues = new wchar_t*[2];
        boolValues[0] = new wchar_t[5];
        boolValues[1] = new wchar_t[6];
        wcscpy(boolValues[0], L"TRUE");
        wcscpy(boolValues[1], L"FALSE");
        pProp = new ConnectionProperty (FDO_RDBMS_CONNECTION_CASESENSITIVE, L"CaseSensitive", L"TRUE", false, false, true, false, false, false, false, 2, (const wchar_t**)boolValues);
        mPropertyDictionary->AddProperty(pProp);

The new values are part of the ConnectionProperty and then added to the mPropertyDictionary.
I guess it's the ConnectionProperty  and/or the mPropertyDictionary that handle this allocated memory.


I will post a new patch with the proper identation in a few minutes


Regards
Bruno
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Bruno Scott
In reply to this post by Andy Zhang
Hi Andy

You're right, the patch was made on the 3.8 Branches.

I'm gone post a new patch based on the trunk, so there will be no traces of that piece of code in it...


Regards
Bruno
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Greg Boone
In reply to this post by Bruno Scott
If there are no further comments....

+1.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Bruno Scott
Sent: Wednesday, September 11, 2013 8:54 AM
To: [hidden email]
Subject: [fdo-internals] RFC 68 for review

Hi all,

RFC 68 (http://trac.osgeo.org/fdo/wiki/FDORfc68) has been posted. It proposes to add case insensitive support to the PostgreSQL provider


Bruno Scott



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/RFC-68-for-review-tp5077290.html
Sent from the FDO Internals mailing list archive at Nabble.com.
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Haris Kurtagic
+1 Haris


On Fri, Sep 20, 2013 at 5:31 PM, Greg Boone <[hidden email]> wrote:
If there are no further comments....

+1.

Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Bruno Scott
Sent: Wednesday, September 11, 2013 8:54 AM
To: [hidden email]
Subject: [fdo-internals] RFC 68 for review

Hi all,

RFC 68 (http://trac.osgeo.org/fdo/wiki/FDORfc68) has been posted. It proposes to add case insensitive support to the PostgreSQL provider


Bruno Scott



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/RFC-68-for-review-tp5077290.html
Sent from the FDO Internals mailing list archive at Nabble.com.
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals


_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Jackie Ng
In reply to this post by Greg Boone
+1 Jackie
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Orest Halustchak
In reply to this post by Haris Kurtagic

+1 Orest

 

From: [hidden email] [mailto:[hidden email]] On Behalf Of Haris Kurtagic
Sent: Saturday, September 21, 2013 5:20 AM
To: FDO Internals Mail List
Subject: Re: [fdo-internals] RFC 68 for review

 

+1 Haris

 

On Fri, Sep 20, 2013 at 5:31 PM, Greg Boone <[hidden email]> wrote:

If there are no further comments....

+1.


Greg

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Bruno Scott

Sent: Wednesday, September 11, 2013 8:54 AM
To: [hidden email]

Subject: [fdo-internals] RFC 68 for review

Hi all,

RFC 68 (http://trac.osgeo.org/fdo/wiki/FDORfc68) has been posted. It proposes to add case insensitive support to the PostgreSQL provider


Bruno Scott



--
View this message in context: http://osgeo-org.1560.x6.nabble.com/RFC-68-for-review-tp5077290.html
Sent from the FDO Internals mailing list archive at Nabble.com.
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals

 


_______________________________________________
fdo-internals mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/fdo-internals
Reply | Threaded
Open this post in threaded view
|

Re: RFC 68 for review

Jackie Ng
In reply to this post by Bruno Scott
Hi Bruno,

We seem to have unanimous agreement to adopt this RFC. Do you need someone to submit these patches on your behalf?

- Jackie