Description: ------------ Behaviour of MySQL and ibase drivers differ (in manager classes): MySQL (expected behaviour): - listTableIndexes() returns only 'INDEX' indexes - listTableConstraints() returns 'PRIMARY' and 'UNIQUE' indexes Firebird: - listTableIndexes() returns 'UNIQUE' indexes (unexpected) - listTableConstraints() returns neither 'PRIMARY' nor 'UNIQUE' indexes (unexpected)

> Firebird: > - listTableIndexes() returns 'UNIQUE' indexes (unexpected) fixed in CVS > - listTableConstraints() returns neither > 'PRIMARY' nor 'UNIQUE' indexes (unexpected) the constraints have to be named with the suffix specified in $options['idxname_format'], otherwise MDB2 filters them out. Don't ask me why. Lukas, what was the reason to do so?

Some RDBMS have issues with allowing people to name indexes like tables etc. This is why we apply the index name conversion, just like we do for sequences. Essentially for databases not created with MDB2 you should set "idxname_format" to "%s" and then all should be well. Maybe we can find a better solution that more gracefully deals with non MDB2 created databases.

I understand that the idxname_format trick may be useful to prevent name clashes during the index/constraint *creation*. What beats me is why should we *list* only the indices created by MDB2 or anyway those conforming to the idxname_format, even if we can easily fetch all the indices created in whatever way/format? To me, it's an unnecessary limit imposed to the user.

I understand. The problem is that the _isIndexName() method serves to purposes similar to the equivalent method for sequences: - check that the name conforms to the format - remove the formatting For sequences this is necessary in order to differentiate with tables names for databases that emulate sequences with tables. So I think we should restrict this behaviour to sequences for databases where we emulate with tables. For all other the _isIndexName() and _isSeqName() should probably always return either the string with the removed formatting or if there was no match the original string.

I have done the necessary changes to CVS.

Okay, I get now the index names almost as expected. But the index names are still truncated (e.g. TEST2_FELD5ID_IDX is returned as test2_feld5id [I know with it is lowercase, that's not the problem]). The naming schema is table name + user defined index name + '_idx' (very old DB_Table behaviour).

I'm sorry, I mixed up the DBMS. Result of ibase usage is: listTableConstraints() returns: array(3) { 0 => string(7) integ_1 1 => string(7) integ_3 2 => string(7) integ_4 } where integ_4 refers to the primary key (can be seen with IBExpert for example). Obviously these are not the real index names. The query has three columns but uses getCol(). But the query itself is the problem: Also RDB$INDEX_NAME AS index_name does not have the index name (it is null for the normal indexes and RDB$PRIMARY2 for the primary key [the last one is okay]). There is one index missing (1 primary, 3 unique). I can see all 4 indexes in IBExpert's table view.

> I understand. The problem is that the _isIndexName() > method serves to purposes similar to the equivalent > method for sequences: > - check that the name conforms to the format > - remove the formatting then _getIndexName() is a probably better name for that method... ;-) A method called _isIndexName() should just return a boolean, IMO. > So I think we should restrict this behaviour to sequences > for databases where we emulate with tables. > For all other the _isIndexName() and _isSeqName() should > probably always return either the string with the > removed formatting or if there was no match the original string. that'd be slightly better, yes. I still think this whole thing is a questionable hack, though, and we're being over-protective towards the user. If the dbms can't have a sequence/index named as a table, then let's return an error and let the user deal with that. my 0.02$

> I have done the necessary changes to CVS. uh-oh... I missed your changes. They look fine :-)

Mark, can you send me your table DDL, please? Either here or by mail. I can't reproduce your issues... I get the correct names in my tests. TIA

Copying the DDL here as well for archive purposes... CREATE TABLE TEST2 ( ID INTEGER NOT NULL, FELD1 DECIMAL(1,0), FELD2 CHAR(15), FELD3 VARCHAR(50), FELD4 SMALLINT, FELD5 INTEGER, FELD6 BIGINT, FELD7 DECIMAL(5,2), FELD8 FLOAT, FELD9 DOUBLE PRECISION, FELD10 BLOB SUB_TYPE 1 SEGMENT SIZE 80, FELD11 DATE, FELD12 TIME, FELD13 TIMESTAMP, ID2 INTEGER NOT NULL ); ALTER TABLE TEST2 ADD PRIMARY KEY (ID2); CREATE UNIQUE INDEX TEST2_FELD5ID_IDX ON TEST2 (ID, FELD5); CREATE UNIQUE INDEX TEST2_FELD5_IDX ON TEST2 (ID); CREATE UNIQUE INDEX TEST2_ID_IDX ON TEST2 (ID);

> CREATE UNIQUE INDEX TEST2_FELD5ID_IDX ON TEST2 (ID, FELD5); > CREATE UNIQUE INDEX TEST2_FELD5_IDX ON TEST2 (ID); > CREATE UNIQUE INDEX TEST2_ID_IDX ON TEST2 (ID); those should have been CONSTRAINTs... anyway, I changed the query and you should get the expected names, now. If not, please re-open this bug report. Thanks for your prompt feedback!

Thanks for the fixes, I get now the names from listTableContraints(). The reason for the usage of "CREATE UNIQUE INDEX" is because DB_Table only knows this "mode" (not my fault *g*). Maybe I'll change that for those DBMS that support such constraints. But (reason for status='open'): The suffix '_idx' is still missing in the output.

Well for that you need to change the "idxname_format" option to "%s" ..

> Well for that you need to change the "idxname_format" option > to "%s" .. If people pass a MDB2 object to DB_Table, this does not seem to be possible. If it is, how?

$format = $db->setOption('idxname_format'); $db->setOption('idxname_format', '%s'); $indexes = $db->manager->listTableIndexes(); $db->setOption('idxname_format', $format);

Strange, yesterday setOption() didn't work for me ("undefined function" in MDB2/Driver/...). But that must have been my mistake then. Thanks Lukas and Lorenzo for the fixes and the help.

While listTableConstraints() works as expected now, listTableConstraintDefinition() (in reverse module) does not: For the 'TEST2_ID_IDX' constraint (index) from the DDL example the following array is returned by the query used in listTableConstraintDefinition(): array(5) { field_name => string(2) ID unique_flag => int 1 foreign_key => NULL description => NULL constraint_type => NULL } Driver/Reverse/ibase.php has the following code starting in line 330: if ($index['unique_flag']) { return $db->raiseError(MDB2_ERROR_NOT_FOUND, null, null, 'getTableConstraintDefinition: it was not specified an existing table constraint'); } Maybe I'm wrong but that's not the behaviour I would expect.

gross... that's probably a leftover from the index/constraint split: http://cvs.php.net/viewcvs.cgi/pear/MDB2/MDB2/Driver/Reverse/ibase.php?view=markup&rev=1.21 Fixed in CVS

While porting the index/constraints methods from MDB2 into DB_Table (for usage with PEAR::DB), I just encountered another problem with the ibase driver: the 'unique' information is still mixed up. Suggested patch: http://www.markwiesemann.de/php/MDB2/MDB2_Reverse_ibase_unique.patch

And another problem (maybe it's me *g*): After adding a primary key field to my table via IBExpert and then calling listTableContraints() and then for the each constraint in the result array getTableConstraintDefinition(), I get this error message for the primary key: > getTableConstraintDefinition: it was not specified an > existing table constraint The constraint name for the primary key is 'rdb$primary2' (from listTableConstraints()) / 'RDB$PRIMARY2' (from IBExpert's constraint view).

the current implementation looks ok to me... can you post the troublesome table IDL, as usual? Thanks!

(Please note that I've reported two bugs today here - one for unique, one for primary keys.) Here is the DDL I used for testing (ID2 is the primary key field for tesing purposes): CREATE TABLE TEST2 ( ID INTEGER NOT NULL, FELD1 SMALLINT, FELD2 VARCHAR(15), FELD3 VARCHAR(50), FELD4 INTEGER, FELD5 INTEGER, FELD6 INTEGER, FELD7 DECIMAL(5,2), FELD8 DOUBLE PRECISION, FELD9 DOUBLE PRECISION, FELD10 BLOB SUB_TYPE 1 SEGMENT SIZE 80, FELD11 DATE, FELD12 TIME, FELD13 TIMESTAMP, ID2 SMALLINT NOT NULL ); ALTER TABLE TEST2 ADD CONSTRAINT TEST2_FELD5ID_IDX UNIQUE (ID, FELD5); ALTER TABLE TEST2 ADD CONSTRAINT TEST2_FELD5_IDX UNIQUE (FELD4); ALTER TABLE TEST2 ADD CONSTRAINT TEST2_ID_IDX UNIQUE (ID); ALTER TABLE TEST2 ADD PRIMARY KEY (ID2); CREATE INDEX TEST2_FELD4_IDX ON TEST2 (FELD4); CREATE INDEX TEST2_NORMALINDEX_IDX ON TEST2 (FELD6, FELD11);

Regarding the PK definition, the issue is not caused by the ibase driver, but by the common implementation of getIndexName(), where '$' is replaced with '_'. Lukas, can we add '$' to the valid chars? Regarding the UNIQUE issue, I don't understand what's the problem... can you spare more details, please? Thanks

> Regarding the UNIQUE issue, I don't understand what's the > problem... > can you spare more details, please? Unique keys are currently partly handled in both getTableConstraintDefinition() and getTableIndexDefinition(), while it should be handled only in the first one (getTableConstraintDefinition). My patch fixes this. (In my test script, ibase driver behaves then the same way as mysql(i) or sqlite.)

what about the current CVS version?

> what about the current CVS version? Good, just one thing is missing: getTableConstraintDefinition() should return the type of the constraint. For primary keys, the type is returned, but not for unique keys. Example from Reverse/mysql.php: if ($row['key_name'] == 'PRIMARY') { $definition['primary'] = true; } else { $definition['unique'] = true; }

This bug has been fixed in CVS. If this was a documentation problem, the fix will appear on pear.php.net by the end of next Sunday (CET). If this was a problem with the pear.php.net website, the change should be live shortly. Otherwise, the fix will appear in the package's next release. Thank you for the report and for helping us make PEAR better.