Opened 7 years ago

Closed 4 years ago

#4567 closed enhancement (fixed)

Ingre Ogr Enhancement

Reported by: zhenchen17 Owned by: warmerdam
Priority: normal Milestone:
Component: OGR_SF Version: unspecified
Severity: normal Keywords: ingres
Cc: Tyler.Mitchell@…

Description (last modified by warmerdam)

This patch enhances Ingres Ogr drivers as follows:

  1. Add two key words to the connection string that are used in database authorization.
      effuser is for the real user in database
      dbpwd is for the password of the effuser
    

It fits the situation that OS authorization user is not the same name with database user name. Ingres requires double authorization check.

  1. Enhance EPSG srid search in database. The previous method is to match the text string representation of a EPSG srid. It is not accuate if there is minor different. Now before a text matching search, a query is execute directly to get the srs_id and confirm:
     SELECT srid FROM spatial_ref_sys WHERE auth_name = 'EPSG' and auth_srid= %s
    
  1. Using system maintained sequence for each fid field in each table instead of the global defined sequence:

notice: this feature is supported since Ingres 9.3

Attachments (6)

ogringresdatasource.cpp.patch (10.6 KB) - added by zhenchen17 7 years ago.
patch file
INGRES.htm (40.3 KB) - added by zhenchen17 7 years ago.
ingres.patch (10.5 KB) - added by warmerdam 7 years ago.
Revised Patch
drv_ingres.html.patch (2.2 KB) - added by zhenchen17 7 years ago.
drv_ingres.html.2.patch (2.2 KB) - added by zhenchen17 7 years ago.
ogringresdatasource.cpp.2.patch (2.3 KB) - added by zhenchen17 7 years ago.

Download all attachments as: .zip

Change History (24)

Changed 7 years ago by zhenchen17

patch file

Changed 7 years ago by zhenchen17

Attachment: INGRES.htm added

comment:1 Changed 7 years ago by warmerdam

Description: modified (diff)
Keywords: ogr removed
Milestone: 1.9.1
Owner: changed from warmerdam@… to warmerdam
Status: newassigned

I have briefly reviewed the patch and it looks pretty good.

I notice that username and password items are also now being pulled from the connection string and this appears to be new, but is not mentioned in the ticket.

I've made a few minor other adjustments and prepared a new patch (attached), which I'd appreciate your reviewing and testing.

Also, I can't apply the INGRES.htm as is. Please prepare a patch or new version of drv_ingres.html and do *not* edit it with an HTML editor that will insert lots of cruft.

As these are new features we will incorporate them in trunk. We can discuss backporting to 1.9 once we are happy with the changes in 2.0 if the changes are necessary for 1.9 to function.

Changed 7 years ago by warmerdam

Attachment: ingres.patch added

Revised Patch

comment:2 Changed 7 years ago by zhenchen17

The patch seems well working by a simple test. It also needs a wide range test by Ingres Ogr user.

I made a patch of Ingres.html(attached). Sorry about the previous version.

The two parameters, username and password, are used in the former version, I didn't change anything except the place where they are used. Actually I added four optional parameters:

  • host : the destination of the connection
  • instance: instance name of Ingres installation
  • effuser: user name in database
  • dbpwd: password used in database for effuser

These parameters are described in the INGRES.html, please check for detail.

Changed 7 years ago by zhenchen17

Attachment: drv_ingres.html.patch added

comment:3 Changed 7 years ago by tmitchell

I will also do a test in a couple of my environments and update the ticket according probably over next two days. Thanks guys for looking at this!

comment:4 Changed 7 years ago by warmerdam

Patches applied in trunk (r24146). Let me know if there are any problems. How critical do you (Chen Zhen or Tyler) feel this is to be in 1.9?

I prefer not to backport new features unless they are critical.

Changed 7 years ago by zhenchen17

Attachment: drv_ingres.html.2.patch added

comment:5 Changed 7 years ago by tmitchell

Thanks Frank, I'll hope to pull it down tomorrow to test.

Re: 1.9 - I should check what GDAL version other apps are using these days. I know QGIS in OSGeo4W was using 1.8. Since your involved with OSGeo4W, which GDAL ver do you expect to use for the next OSGeo4W (or FWTools for that matter) release?

It would be ideal if we could at least use it out of the box in a few apps there without a real version mismatch. Thoughts?

Not in a rush to get backport support either way. I'm used to running from trunk, but at some point I have to point Ingres users to something and the more 3rd part apps that can use it easily, the better. Hope that makes sense.

comment:6 Changed 7 years ago by warmerdam

Tyler,

I just packaged GDAL 1.9 for OSGeo4W and Jurgen mentions he will be updating fairly soon.

I mostly mentioned 1.9 because Chen originally had a milestone of GDAL 1.9 on this ticket which I removed.

comment:7 Changed 7 years ago by zhenchen17

Frank and Tyler:

I didn't mean 1.9 by purpose. I think it would be in the next release(1.9.1? or 2.0?). It sounds reasonable to cooperate with old version GDAL by plugin mode and build-in after the next release.

comment:8 Changed 7 years ago by tmitchell

I'm fine with that Chen, handling as plugin later on is reasonable I think.

I've built your changes fine with latest changes on RHEL. I tested #2 - EPSG usage works great. A really nice improvement (now I can simplify some of my docs :) ).

I'm running some more tests to verify #1 and #3 - more to come.

comment:9 Changed 7 years ago by tmitchell

[Sorry about format of number in last comment, I didn't mean to use syntax for referring to other tickets :)]

I'm testing your #3 - re: sequences and hitting bad behaviour. When trying to load my second table, it tries to create the new sequence but using same name as the first:

CREATE SEQUENCE: The sequence 'seq_ogr_fid' already exists. STATEMENT aborted.

comment:10 in reply to:  9 Changed 7 years ago by zhenchen17

Replying to tmitchell:

[Sorry about format of number in last comment, I didn't mean to use syntax for referring to other tickets :)]

I'm testing your #3 - re: sequences and hitting bad behaviour. When trying to load my second table, it tries to create the new sequence but using same name as the first:

CREATE SEQUENCE: The sequence 'seq_ogr_fid' already exists. STATEMENT aborted.

A sequence could related to any table. So it is global and would not be deleted when tables delete.

The ways to solve is :

  1. Deleting the sequence when the geometry table is deleted. It requires the special connection between table name and sequence name. If the table is created by handle, there will be a mis-deleting.
  1. Do not specify the sequence name in "Create Table" statement and let system assign a random one. I don't like that because it would be confused after a lot of operation.

What is your idea?

comment:11 Changed 7 years ago by warmerdam

Chen- can you expand on what happens if you don't provide a name for the sequence in the create table? Why would this be confusing after a lot of operations?

BTW, what was wrong with the old behavior of using one global sequence? Did you just not like the fact that fid's didn't start at one for each table?

comment:12 in reply to:  11 Changed 7 years ago by zhenchen17

Replying to warmerdam:

Chen- can you expand on what happens if you don't provide a name for the sequence in the create table? Why would this be confusing after a lot of operations?

BTW, what was wrong with the old behavior of using one global sequence? Did you just not like the fact that fid's didn't start at one for each table?

If the sequence name is not specified in the create statement

CREATE TABLE table_name (fidINTEGER NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS   IDENTITY (START WITH 1 INCREMENT BY 1)

a sequence will be created automatically based on rules, r1234BACD, for example. I guess it is hex form of the table id and column id. Once the table is deleted, the sequence is left and become meaningless. Maybe it could be call garbage.

I just feel it strange to start from a very huge fid in a table. Though the function of fid is the same.

Any idea?

comment:13 Changed 7 years ago by tmitchell

I could go either way on this. I prefer proper garbage collection - so your first solution would make that easiest. Randomly named sequences seems less useful to me, let's avoid it for now :) Make sense?

comment:14 in reply to:  13 Changed 7 years ago by zhenchen17

Replying to tmitchell:

I could go either way on this. I prefer proper garbage collection - so your first solution would make that easiest. Randomly named sequences seems less useful to me, let's avoid it for now :) Make sense?

Approve. When dropping a geometry table in Ogr, the related sequence will be deleted too. If the table is deleted by hand, (execute sql in vdba etc.), the sequence would be garbage.

comment:15 Changed 7 years ago by zhenchen17

Sorry for my misunderstand for the sequence management.

In version 10, the sequence created by create statement seems to be managed by system, which means it will be deleted when the table deleted. It seems to be an improvement for 9.3.

So no need to delete the sequence by hand. :-)

Also a patch is attached to fix a name confusing bug when creating the table. Just pay attention to if the table is created but ignore the name of the field.

Changed 7 years ago by zhenchen17

comment:16 Changed 6 years ago by warmerdam

Last little patch applied in trunk (r24341).

comment:17 Changed 6 years ago by warmerdam

1.9 upgraded to match trunk for ingres driver (r24349)

comment:18 Changed 4 years ago by Even Rouault

Resolution: fixed
Status: assignedclosed

Looks like this ticket could be closed.

Note: See TracTickets for help on using tickets.