Problems in the field editor with fields of the same name in two different tables

[Fr0sT]

Hello Jan,

This is what we propose for Firebird in Zeos 7.2. If I understood you correctly, the problem is, that a caller calls GetColumns with the tablename quoted -> "TableName" instead of TableName. So for Zeos 7.2 the Firebird driver should remove the quotes if they are supplied before the value is used for any comparisons. This should solver the problem there too?

As far as I understood there's no FB-specific methods in the call chain at all. Everything that deals with query parsing (that's where quotes come from) and field creating is generic. So - "all or nothing". I guess this issue is neither too critical nor frequent so there's no big trouble if 7.2 will remain unchanged. Quoted identifiers are evil anyway.

[tkszeos]

I think that the ability to correctly manage quoted field names is an important feature to be implemented in ZeosLib, also because other components do it without problems. I can agree with Fr0sT that it is not advisable to use them but it is also true that it is not prohibited. In fact, Firebird allows it. I hope this feature can soon be implemented also because it would solve many problems for me. I have been using ZeosLib for many years and have never had any problems.
Thank you for your great work.

[Fr0sT]

So - what's the final decision?
As fas as I'm concerned, the problem completely lies in generic (for all drivers) methods. I found no FB-specific methods that could be modified to resolve the issue.

[marsupilami]

Hello Fr0st,

I had a look into the problem and have to admint the current code is a mess While all you have said is true, the real problem for me is that TZAbstractResultSetMetadata.LoadColumn overwrites the TZColumnInfo without regarding what information already is there. Even worse - it simply destroys everything because it calls ReadColumnByRef which in turn clears the TZColumnInfo on its first line.

So my thought was to try to keep the information and assuming that everything was alright if the table name and the column name already have been set. It turned out that this uncovers another problem: TZInterbase6XSQLDAResultSet.Open doesn't set Writable and Definitlywritable. So I solved that one too.

This still raises sume bugs in the test suites but I am not sure wether that are problems in the tests or problems in the library. I already fixed one test.

I created a patch file for Zeos 7.2 and attached it. Let me know what you think

Jan

[Fr0sT]

Wow, this is pretty smart!
So FB code seems to completely ignore metadata when creating ColumnInfo objects? I'd say it's not a right way. I think it needs a redesign to use metadata. It also will help to implement domain-based GUID type assignment. I'll do some experiments in this area

[marsupilami]

Well - it is more like the generic layers of the DBC layer don't trust the lower, driver specific layers to do their work correctly. I thought about having some mechanism to inform the generic parts, which info in TZColumnInfo already could be determined by the driver specific part and which couldn't. I don't expect that all driverc can fill out TZColumnInfo in the open event correctly (yet).

I assume trhat not all drivers could deliver correct information in the TZColumnINfo at some time and that this is the reason why there was the decision to try to get all the informatioon using GetColumns and the like.

[EgonHugeist]

Hi all,

i had a talk with Jan yesterday. I'm against the proposed patch.
This patch try's to fix the reported issue in a generic way which is wrong from my POV.

Before going into more details:

First:
There is a component TZMetaData?. This component uses the Dbc metainformation. Currently it seem's this component isn't realy tested in our suits. But from what i see (if there would be extended tests using this component), most tests would fail because trying to fix field-types/names/etc for the component layer by simply skipping updating the column in the metadata-resultsets. This is wrong. All informations should be available.

In some circumstances this makes trouble like the report of tkszeos or just think about:

Code: Select all

select cast(my_int_field) as varchar) as Number from myTable 

The column-informations of the native resultset and the metainformations for the table "myTable" will never fit..

Second:
I had a look to the current code. The TZAbstractResultSetMetadata offers all we need. Almost all methods are virtual. Such as:
TZAbstractResultSetMetadata.ClearColumn
TZAbstractResultSetMetadata.GetTableName .. All

From my POV the missing point here are driver based descandants of TZAbstractResultSetMetadata which resolve all the ResultsetMetaData settings for the IZResultSetMetadata in it's own way.
Drivers like FireBird are pretty powerfull on decribing the bound columns whereas thinking about Access or SQLite(ColumnTypes) we need a crystal ball OR the Metainformations .

In addition current code like

Code: Select all

function TZAbstractResultSetMetadata.GetTableName(ColumnIndex: Integer): string;
begin
  if not Loaded then
     LoadColumns;
  Result := TZColumnInfo(FResultSet.ColumnsInfo[ColumnIndex {$IFNDEF GENERIC_INDEX}-1{$ENDIF}]).TableName;
end;

forces always to load columninformations via retriving the metainformations.. Is this neccessary in all cases? F.e. in a ReadOnly+ForwardOnly mode as well as scrollable/updatable resultsets? I don't think so.

I'd like to vote for:
Keep away trying to resolve such issues in a generic way. It's not possible.
ALLWAYS try to complete the cached dbc-metainformations for all drivers!
Introducing TZAbstractResultSetMetadata descendants for the Drivers. Some are done but i'ts code seems to be incomplete too. e.g. MySQL

Edit: another handy solution could be a driver based TZColumninfo. Just think about if the fields where assigned by using virtual Setters and Getter methods. A TZFBColumnInfo could dou something like:

Code: Select all

procedure TZFBColumnInfo.SetTableName(const Value: String);
begin
  if (GetTableName = '') and (Value <> '' then
    inherited SetTableName;
  end; 

I know it means loads more work 4 us. What you guys think?
I could start an example if you want..

Cheers, Michael

[Fr0sT]

I'm now examining the corresponding metadata/columninfo code and tend to agree. Currently there's a weird situation: RS.Open queries column data (probably incomplete/incorrect) and then occasionally LoadColumns is called that invokes Metadata query which loads correct column info for the current statement and then... aww, some things are taken from what RS.Open has assigned and some from metadata and the brain just blows down into pieces.

My guess of what should be: RS.Open > Metadata query > Fill column info.

OTOH, this change is somewhat global while Jan's patch seem to solve the issue with minimal efforts.

ADDED

I think I got the idea...
- RS.Open - inits ColumnInfo with data it received by executing a statement
- Dataset.InternalOpen > InternalInitFieldDefs - utilizes ColumnInfo data, then checks IsReadOnly/Writable/etc which calls metadata.LoadColumns
- Metadata.LoadColumns - queries metadata and overrides ColumnInfo completely.

My guess on how it should be:
- RS.Open - inits ColumnInfo with data it received by executing a statement
- Dataset.InternalOpen > InternalInitFieldDefs - calls metadata.LoadColumns
- Metadata.LoadColumns - queries metadata and extends (optionally overrides) ColumnInfo - without ClearColumn.
- InternalInitFieldDefs - utilizes final ColumnInfo data

[Fr0sT]

Test change: disable ClearColumn and call metadata loading before field creation. Could you guys run test suite with this modification?

Code: Select all

 src/component/ZAbstractRODataset.pas | 4 ++++
 src/dbc/ZDbcResultSetMetadata.pas    | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/component/ZAbstractRODataset.pas b/src/component/ZAbstractRODataset.pas
index 63f825b..bc8c69a 100644
--- a/src/component/ZAbstractRODataset.pas
+++ b/src/component/ZAbstractRODataset.pas
@@ -3269,6 +3269,10 @@ begin
 
     with ResultSet.GetMetadata do
     begin
+
+    // ** TEST ** - force metadata load and fill column props over existing ones
+    IsWritable(FirstDbcIndex);
+
     ConSettings := ResultSet.GetConSettings;
     if GetColumnCount > 0 then
       for I := FirstDbcIndex to GetColumnCount{$IFDEF GENERIC_INDEX}-1{$ENDIF} do
diff --git a/src/dbc/ZDbcResultSetMetadata.pas b/src/dbc/ZDbcResultSetMetadata.pas
index bade232..dd256b2 100644
--- a/src/dbc/ZDbcResultSetMetadata.pas
+++ b/src/dbc/ZDbcResultSetMetadata.pas
@@ -778,7 +778,8 @@ function TZAbstractResultSetMetadata.ReadColumnByRef(
   FieldRef: TZFieldRef; ColumnInfo: TZColumnInfo): Boolean;
 begin
   Result := False;
-  ClearColumn(ColumnInfo);
+  // ** TEST - do not delete already assigned props. Probably should remain for early exits
+  //ClearColumn(ColumnInfo);
   { Checks for uncompleted field reference. }
   if not Assigned(FieldRef) or not Assigned(FieldRef.TableRef) then
     Exit;

[EgonHugeist]

@FrOst

could you please move an generic example to the test-suites?
Your patch didn't fix the issue, and loads of more protocols raising error and more fails..

@All

iv'e attached an proposal to get rid of that issue. Honestly i was out of time to test the example of tkszeos.
The patch minimizes metadata roundtrips for pure dbc users like me and should also handle the reported problem. It's not 100% complete.

The way i did it:
1. do not flush Columninfos if we know the driver is able to determine them correctly.
2. Add an comparision if the FieldRef of TableRef fit's to the previous determined columninfo by compare Catalog, Schema, TableName.
Note that there is a missing MySql point see commit 4115. MySQL is able to use case sensitive identifiers for all objects except tables where it depends to a global setting (mysql stores each table as files optional case (in) sensitive).
3. Add missing overrides to the protocol descendants to avoid running into the LoadColumns trap what i really don't need it.

Finally we need a generic example. Postgres as well as Oracle(may be, but how?) are not able to describe the real tablename of a fetched column in the NativeResultsets without more roundtrips. All others including SQLite are doing that very well.
As far as i remember Jan wrote an InfoCache for PG. Which could be an full replacement for the MetaInfos?
My impression is like FrOst's: current code doesn't trust the generic Column-Informations and overwrites everything.

Please test my patch and give a reply.

[Fr0sT]

Michael, yep, my quick fix was too quick. There are several comparisons that were true after column clearing and they executed some necessary actions. I tried commenting some conditions out and tests succeeded though another tests started failing

[EgonHugeist]

Not a problem, FrOsT. OpenSource is an open discussion and no one known who wins it. Thank for your help btw!

It would be nice if someone can post his findings. If all do agree .. we'll comit the patch. Inbetween i'll add a test running against the known tables.
Something like

Code: Select all

select dep_id as dep_name, dep_name as dep_id from department;

IIRC such a name exchange could make trouble too.

[EgonHugeist]

Double post as Notification...

@tkszeos

please update from \testing-7.2, patch done R4126. This should fix your issue. Note fastes way for Column to Table determination is using a Alias or append the TableName to the fieldname.

@Jan&FrOsT,

The quoting rules are damn ugly. Propose to omit the DecomposeObjectString function in the uncached Metadata functions.
If the IZSelectSchema determines the TZFieldRefs all Identifiers schould be dequoted, TableRefs determined and all pattern fields for the Metadata should work with unquoted patterns only. On forming a statment the quoting rules should be used same as a user should do it.
Current code tests everything a bit paranoid for Quoted/Unquoted LeftSet <> Quoted/Unquoted RightSet to determine "something".
For me this is buggy and somewhat error prone! After the patch the current code is able to resolve most things but as long there are no clear docs about it i propose to raise a ticket.

What do you think?

[Fr0sT]

Michael, nice and big work!
1) "the generic resolver should add Catalog and Schema only of Supported" - hmm, could there really be a case when Schema or Catalog <> '' but driver doesn't support these properties?
2) Links "http://zeoslib.sourceforge.net/viewtopi ... sid=97f200..." contain sid parameter which is session ID and is useless
3) In TZSelectSchema.LinkFieldByIndexAndShortName:
- Convertor.Quote(Field) value is used pretty frequently to be cached
- Construction

Code: Select all

  {$IFDEF GENERIC_INDEX}
  if (ColumnIndex >= 0) and (ColumnIndex < FFields.Count) then
  {$ELSE}
  if (ColumnIndex > 0) and (ColumnIndex <= FFields.Count) then
  {$ENDIF}

could be replaced with

Code: Select all

  if (ColumnIndex >= FirstDbcIndex) and (ColumnIndex <= FFields.Count {$IFDEF GENERIC_INDEX} - 1 {$ENDIF}) then

If the IZSelectSchema determines the TZFieldRefs all Identifiers schould be dequoted, TableRefs determined and all pattern fields for the Metadata should work with unquoted patterns only. On forming a statment the quoting rules should be used same as a user would do it.

Totally agreed! This quoting madness makes me crazy. Zeos should operate with plain names internally, using quotes only when constructing SQL queries.

Propose to omit the DecomposeXXX function in the uncached Metadata functions.

I tend to agree (some of overrides are 1:1 as parent one) but some overrides do quote identifiers in these methods. Omitting them will break these drivers.

[EgonHugeist]

1) "the generic resolver should add Catalog and Schema only of Supported" - hmm, could there really be a case when Schema or Catalog <> '' but driver doesn't support these properties?

Yep this is version related using MySQL f.e. There are folks like Sphinx which use the mySQL protocols but they do not support Catalog or schema but MySQL since 3.5 is able to use the "Catalog" (usually it's the schema but JDBC maps it as Catalog). ODBC, OleDB and ADO (don't know how) support same syntax..

2) Links "http://zeoslib.sourceforge.net/viewtopi ... sid=97f200..." contain sid parameter which is session ID and is useless

Lorbs...

- Construction

Code: Select all
{$IFDEF GENERIC_INDEX}
if (ColumnIndex >= 0) and (ColumnIndex < FFields.Count) then
{$ELSE}
if (ColumnIndex > 0) and (ColumnIndex <= FFields.Count) then
{$ENDIF} [/code]

Do it! I've noticed you did a nice simplification on my ugly code

I tend to agree (some of overrides are 1:1 as parent one) but some overrides do quote identifiers in these methods. Omitting them will break these drivers.

Will it? Needs to be tested. And this job can't be made in atomic commits. Either all or nothing..
In this case not all can work on it... volunteers ?

Or by using patches as interface..

If all do agree, of course.