Moodle

auth_sync_users() only knows mysql and postgres

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.7
  • Fix Version/s: 1.8, 1.9
  • Component/s: Authentication
  • Labels:
    None
  • Environment:
  • Database:
    Microsoft SQL, Oracle
  • Affected Branches:
    MOODLE_17_STABLE
  • Fixed Branches:
    MOODLE_18_STABLE, MOODLE_19_STABLE

Description

auth_sync_users() in auth/ldap/lib.php needs to create a temporary table so that it can quickly determine which external ldap accounts exist or don't exist in moodle. The code currently only knows how to create temporary tables for mysql and postgres. This patch should add support for doing it in mssql and oracle. A few comments:

-this is my first patch submission attempt so go easy on me, I'd really appreciate constructive criticism
-I looked but was unable to find a way to abstract the temporary table creation with adodb, if anyone knows of a way, it would be much prettier than the switch case statement I have now. As it stands, this will need updating with every new database system moodle supports.
-It seemed to me that mssql was not dropping the temporary table when I ran the script multiple times within a few minutes of each other so I added the DROP table command at the end to be sure each run would be working with fresh data.

Issue Links

This issue will be resolved by:
MDL-8590 Auth cleanup - META Blocker Closed
This issue is duplicated by:
MDL-8153 LDAP authentication (sync_users) doesn't support MSSQL and Oracle Major Closed
MDL-8023 CLONE -auth_ldap_bulk_insert() potential problem for MSSQL and Oracle Minor Closed
This issue has a non-specific relationship to:
MDL-8153 LDAP authentication (sync_users) doesn't support MSSQL and Oracle Major Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jay Lee added a comment -

Also, for MySQL databases, SET SQL_BIG_TABLES=1 should no longer be necessary. It was needed for MySQL 3.x since MySQL would use memory based temp tables by default but would report an error if the table exceeded a memory limitation. As of MySQL 4.0, temporary tables will be memory based until such a time as they grow larger than the memory allows, at that point they will be written to disk without causing any errors. We should leave SET SQL_BIG_TABLES unset so that MySQL can use memory for the temporary table when it can. Further details can be found in MySQL's docs at:

http://dev.mysql.com/doc/refman/4.1/en/set-option.html#id2832220

Show
Jay Lee added a comment - Also, for MySQL databases, SET SQL_BIG_TABLES=1 should no longer be necessary. It was needed for MySQL 3.x since MySQL would use memory based temp tables by default but would report an error if the table exceeded a memory limitation. As of MySQL 4.0, temporary tables will be memory based until such a time as they grow larger than the memory allows, at that point they will be written to disk without causing any errors. We should leave SET SQL_BIG_TABLES unset so that MySQL can use memory for the temporary table when it can. Further details can be found in MySQL's docs at: http://dev.mysql.com/doc/refman/4.1/en/set-option.html#id2832220
Hide
Permalink
Jay Lee added a comment -

I have also confirmed that Oracle should also have:

$bulk_insert_records = 1;

apparently multiple row inserts is a MySQL only hack (but a darn nice one I must say). MSSQL, Postgres and Oracle do not support it and inserts must be done one row at a time.

Jay

Show
Jay Lee added a comment - I have also confirmed that Oracle should also have: $bulk_insert_records = 1; apparently multiple row inserts is a MySQL only hack (but a darn nice one I must say). MSSQL, Postgres and Oracle do not support it and inserts must be done one row at a time. Jay
Hide
Permalink
Iñaki Arenaza added a comment -

As MySQL is the only special case, the patch would be far easier if you just special-cased MySQL.

On the other hand, I don't understand why you use a special table name beginning with '#' just for MS SQL. Does it have any special requirements with respect to table names?

Saludos. Iñaki.

Show
Iñaki Arenaza added a comment - As MySQL is the only special case, the patch would be far easier if you just special-cased MySQL. On the other hand, I don't understand why you use a special table name beginning with '#' just for MS SQL. Does it have any special requirements with respect to table names? Saludos. Iñaki.
Hide
Permalink
Iñaki Arenaza added a comment -

Never mind the comment above. Table creation is quite different for each db, so the patch won't be any easier.

On the other hand, if temporary table creation is database dependant, I'd say support for this should go in lib/ddllib.php (or lib/datalib.php in 1.6 and below)

Saludos. Iñaki.

Show
Iñaki Arenaza added a comment - Never mind the comment above. Table creation is quite different for each db, so the patch won't be any easier. On the other hand, if temporary table creation is database dependant, I'd say support for this should go in lib/ddllib.php (or lib/datalib.php in 1.6 and below) Saludos. Iñaki.
Hide
Permalink
Jay Lee added a comment -

> Never mind the comment above. Table creation is quite different for each db, so the patch won't be any easier.

Yes, they are all quite unique, MSSQL for example uses a # in front of the name in order to indicate it is a temporary table.

> On the other hand, if temporary table creation is database dependant, I'd say support for this should go in lib/ddllib.php (or lib/datalib.php in 1.6 and
> below)

I can't really comment on this as I'm not familiar with the overall layout of Moodle's source. The included patch "just works" for me and I'd really like to have it available by the time we go live with our MSSQL deployment. Has anybody tested the patch yet?

Jay

Show
Jay Lee added a comment - > Never mind the comment above. Table creation is quite different for each db, so the patch won't be any easier. Yes, they are all quite unique, MSSQL for example uses a # in front of the name in order to indicate it is a temporary table. > On the other hand, if temporary table creation is database dependant, I'd say support for this should go in lib/ddllib.php (or lib/datalib.php in 1.6 and > below) I can't really comment on this as I'm not familiar with the overall layout of Moodle's source. The included patch "just works" for me and I'd really like to have it available by the time we go live with our MSSQL deployment. Has anybody tested the patch yet? Jay
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Hi Jay, thanks so much for you nice patch. I've added it to 18_STABLE and HEAD.

I've added some minor modifications on order to guarantee temp tables to be dropped, because it can cause problems under "persistent" environments.

Could somebody test its working under MSSQL and Oracle? My Oracle box is broken and I haven't LDAP here (although the patch hasn't modified any related to LDAP).

Show
Eloy Lafuente (stronk7) added a comment - Hi Jay, thanks so much for you nice patch. I've added it to 18_STABLE and HEAD. I've added some minor modifications on order to guarantee temp tables to be dropped, because it can cause problems under "persistent" environments. Could somebody test its working under MSSQL and Oracle? My Oracle box is broken and I haven't LDAP here (although the patch hasn't modified any related to LDAP).
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Confirmed under MSSQL. Only Oracle is pending....

Show
Eloy Lafuente (stronk7) added a comment - Confirmed under MSSQL. Only Oracle is pending....
Hide
Permalink
Jay Lee added a comment -

I have setup a vmware Centos4 test LAMP image and am getting ready to test against MSSQL and Oracle Express versions and our eDirectory LDAP tree of ~1500 users.

Show
Jay Lee added a comment - I have setup a vmware Centos4 test LAMP image and am getting ready to test against MSSQL and Oracle Express versions and our eDirectory LDAP tree of ~1500 users.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Great Jay,

I cannot test under Oracle because those $%&%$ Oracle guys haven't any client available for my cool (and useless here ) Intel MacOS X.

Any feedback will be welcome! Thanks for your support! B-)

Show
Eloy Lafuente (stronk7) added a comment - Great Jay, I cannot test under Oracle because those $%&%$ Oracle guys haven't any client available for my cool (and useless here ) Intel MacOS X. Any feedback will be welcome! Thanks for your support! B-)
Hide
Permalink
Jay Lee added a comment -

OK, I can verify the patch is working for MSSQL. I'm still trying to get my Oracle setup working again in order to test it (really Eloy, you should be glad the haven't ported such a pain in the butt sql client over to OSX86). I'll see if I can get it up tomorrow and report back here but I'm fairly certain we should be good with this one now.

Show
Jay Lee added a comment - OK, I can verify the patch is working for MSSQL. I'm still trying to get my Oracle setup working again in order to test it (really Eloy, you should be glad the haven't ported such a pain in the butt sql client over to OSX86). I'll see if I can get it up tomorrow and report back here but I'm fairly certain we should be good with this one now.
Hide
Permalink
Jay Lee added a comment -

Doesn't seem to be working correctly on Oracle with latest CVS code. There appears to be some other LDAP changes going into the 1.8 stable branch that might have affected my testing though.

Is there an easy way to have Moodle print out the SQL statements it is issuing so I can take a look at them (and their order)?

Jay

Show
Jay Lee added a comment - Doesn't seem to be working correctly on Oracle with latest CVS code. There appears to be some other LDAP changes going into the 1.8 stable branch that might have affected my testing though. Is there an easy way to have Moodle print out the SQL statements it is issuing so I can take a look at them (and their order)? Jay
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Uhm,

I think you can add:

global $db;
$db->debug = true;

at the beginning of sync_users()

and then, change all the calls to execute_sql() and execute_sql_arr() to have their last parameter (feedback) set to true. It should produce all the SQL statements to be printed on screen.

Thanks!

Show
Eloy Lafuente (stronk7) added a comment - Uhm, I think you can add: global $db; $db->debug = true; at the beginning of sync_users() and then, change all the calls to execute_sql() and execute_sql_arr() to have their last parameter (feedback) set to true. It should produce all the SQL statements to be printed on screen. Thanks!
Hide
Permalink
Petr Škoda (skodak) added a comment -

closing now,
please file a new report for problems in 1.8 branch if needed

thanks for report and testing!

Show
Petr Škoda (skodak) added a comment - closing now, please file a new report for problems in 1.8 branch if needed thanks for report and testing!

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.