Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IBM DB2 iSeries support #910

Closed
wants to merge 29 commits into from
Closed

Add IBM DB2 iSeries support #910

wants to merge 29 commits into from

Conversation

cassvail
Copy link

IBM iSeries DB2 has a lot of differences from the linux version.
This set of commits gets dbal and even base functionalities of doctrine-orm to work on IBMi systems without breaking the base set of DB2 classes.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1304

We use Jira to track the state of pull requests and the versions they got
included in.

*
* @return resource
*/
public function getConn() //getWrappedResourceHandle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably not be implemented

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the same behaviour as MysqliConnection->getWrappedResourceHandle()
On IBMi systems is necessary to have access to the resource object to use the native XMLToolkit which is a php library used to execute IBMi programs, change system libraries, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably keep the same naming then, but still, we'd need a specific use-case example for this. I don't see why a user can't simply build a new connection object rather than relying on doctrine's.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the method name.
Here is the Zend XMLToolkit docs using directly the resource handler.
http://files.zend.com/help/Zend-Server-6-IBMi/content/toolkit_service_class.htm
The fact is on IBMi system once you get a connection to the system it's not just a simple connection but a lot of system services are created and bound to it.
For example a temporary schema (QTEMP) which is avaiable only using that connection, so a SQL statement won't be able to access temporary tables created by another connection (the case you proposed).
Another example is about Library Lists on IBMi. If you use the connection parameter i5_naming=system, the system will look for a table not on a single schema but on a list of schemas, using the first match.
If a program or a XMLTookit command is executed to change the library list, it will affect which table is queried. This is also bound to the connection, so another connection resource wouldn't reflect the system changes.
I hope this could answer to your doubts. I know IBMi system kind of complex and unfortunately not so standard.

@asgrim
Copy link
Contributor

asgrim commented Dec 7, 2015

@cassvail any idea if you'll be able to add test coverage for this?

@cassvail
Copy link
Author

cassvail commented Dec 8, 2015

Yes. I was waiting for more feedback on the code. I'll be working on it as soon as I can.

@deeky666
Copy link
Member

@cassvail first of all thanks for your contribution. As always the "problem" with newly introduced platforms, we need someone who maintains it if we decide to introduce it in the core. Unfortunately with platforms like this, it is quite hard for us to maintain as nobody has decent knowledge about it and it's not easily testable. So we would require you to actively maintain it, otherwise we cannot accept it for the given reasons :( That's a decision you have to make...

@cassvail
Copy link
Author

@deeky666 In my company we have different projects on IBMi system running PHP and using the dbal module (currently my ibmi branch). So I can tell it's also in our interest to have it running correctly and for this reason we will keep our branch updated.
As you already know it's not that easy to test on IBMi environment mainly because of the many differences in the OS and db engine, but as we work on it, we can do our best to keep the dbal support running for the IBMi platform. In fact our branch mostly uses the logics from the Linux DB2 Platform which is already supported.
That seemed to me a good chance to contribute to the project and the community.

$over = 'ORDER BY ' . implode(', ', $overColumns);
}

$sql = 'SELECT DOCTRINE_TBL.* FROM (SELECT ROW_NUMBER() OVER('.$over.') AS DOCTRINE_ROWNUM, DOCTRINE_TBL1.* '.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the DOCTRINE_TBL1.* needs to come before DOCTRINE_ROWNUM, as Doctrine Migrations casts the result of a query using this to a string, the additional column causes the "current migration" to be 1 instead of the correct version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cassvail I've made a PR for your branch: cassvail#1 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cassvail unit test failure introduced should be fixed in cassvail#2

@deeky666
Copy link
Member

@cassvail sounds promising. That my also be an opportunity for us to gather some more advanced DB2 knowledge, because that is still rather limited by the members in the core team here, thus having a rather "unstable" DB2 platform in Doctrine right now. @zeroedin-bill @Ocramius thoughts?

@billschaller
Copy link
Member

@deeky666 A better driver is great, but unless we have a way to run tests regularly, it's going to be hard to keep regressions out.

cassvail and others added 2 commits December 12, 2015 17:55
Change order of parameters in doModifyLimitQuery to work with Migrations
@Ocramius
Copy link
Member

@deeky666 I'd suggest onboarding somebody that has IBMi knowledge, then passing the token to that person once this is merged.

The alternate solution is to have this as a separate library, but then the usability is kinda terrible (not that IBMi has ever been developer friendly)

Fixes unit test DB2PlatformTest::testModifiesLimitQuery
@asgrim
Copy link
Contributor

asgrim commented Jan 25, 2016

@cassvail it's probably worth rebasing/merging as #2310 is now merged which contains the fixes I already made in this branch btw

@asgrim
Copy link
Contributor

asgrim commented Apr 4, 2016

@cassvail I have a problem with a column in the ORDER BY not bubbling up correctly:

SELECT DOCTRINE_TBL.*
FROM (
  SELECT DOCTRINE_TBL1.*, ROW_NUMBER() OVER(ORDER BY m1_.date_created DESC) AS DOCTRINE_ROWNUM
  FROM (
    SELECT m0_.id AS ID_0, m0_.answer_index AS ANSWER_INDEX_1, m0_.answer_value AS ANSWER_VALUE_2, m0_.submission_id AS SUBMISSION_ID_3, m0_.question_element_id AS QUESTION_ELEMENT_ID_4
    FROM MEDICAL_FORM_ANSWER m0_
      INNER JOIN MEDICAL_FORM_SUBMISSION m1_ ON (m0_.submission_id = m1_.id)
      INNER JOIN PERSON p2_ ON (m1_.person_id = p2_.id)
    WHERE p2_.application_id = ?
      AND m0_.question_element_id = ?
      AND m0_.answer_index = ?
    ORDER BY m1_.date_created DESC
  ) DOCTRINE_TBL1
) DOCTRINE_TBL
WHERE DOCTRINE_TBL.DOCTRINE_ROWNUM BETWEEN 1 AND 1

The error I see is:

SQLSTATE=42703 SQLCODE=-206 Column or global variable DATE_CREATED not found.

I'm able to fix manually by changing OVER(ORDER BY DOCTRINE_TBL1.date_created DESC) and adding m1_.date_created to the innermost SELECT, but I'm not sure how to implement this cleanly into your doModifyLimitQuery method, as it would involve modifying the innertmost $query provided... additionally, if I do a fetch-join (to force m1_ to be included in the select), the column name becomes aliased as DATE_CREATED_4 which makes it actually more complex :/ any suggestions?

@cassvail
Copy link
Author

cassvail commented Apr 6, 2016

@asgrim About your problem with a column in the ORDER BY not bubbling up correctly.
At the moment we couldn't find different solutions, because the OVER statement requires the column to be present in the inner result set, so as you did we always need to SELECT the interested column in the inner query.
This is SQL Engine problem on IBMi as is the ORDER BY not bubbling up on sub-queries, I think I could give a look if any newer solutions have came up, but I don't think so.

@asgrim
Copy link
Contributor

asgrim commented Apr 6, 2016

@cassvail I believe newer DB2 fixes introduce LIMIT / OFFSET correctly, although I've only heard this third party, that might be worth investigating to simplify this logic?

@cassvail
Copy link
Author

cassvail commented May 2, 2016

@asgrim I've been doing some PHPUnit tests on the IBMi SQL Engine. Here is my gist:
https://gist.github.com/cassvail/76e0d65774d20e2d67adae6b7c301afa
My tests are passing on a V7R2, not the latest IBMi version (because a few weeks ago the V7R3 came out) but both the nested ORDER BY and LIMIT OFFSET problems persist.
About the last one, as stated on IBM website there are some Technology Refresh available to have LIMIT OFFSET support https://www.ibm.com/developerworks/community/wikis/home?lang=en#!/wiki/IBM%20i%20Technology%20Updates/page/IBM%20i%207.1%20-%20TR11%20Enhancements
Those TR must be installed on the machine and I wouldn't be so confident that all the IBMi systems are updated so frequently and with those specific PTF. Thus I don't think it would be wise to do any major fix at the moment on those subjects.
Still I'll be doing some Unit Tests on the IBMiPlatform to see if we can simplify the logic without changing the behaviour

@morozov morozov closed this Nov 30, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants