In C4::Members, there is an unused subroutine, columns(), which uses "SHOW COLUMNS FROM" to get the columns for borrowers. This subroutine is unused currently. It would be best to remove it, and reimplement later, using database independent logic, should we actually need it.
Created attachment 8617 [details] [review] Remove the unused columns() function, which uses a MySQLism
Created attachment 8874 [details] [review] Remove the unused columns() function, which uses a MySQLism Signed off
QA comment: I checked that the column() function is unused by using: grep -R "columns(" * I could only find some _get_columns() calls, that are different passed QA
OK, now it's really pushed: http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=1c8df08aea5eb173d7216cdfc9895fdd601a5f11
It appears, it is used :-/ I have an error on tools/import_borrowers.pl: Software error: Can't locate object method "columns" via package "C4::Members" at /.../tools/import_borrowers.pl line 61.
(In reply to comment #5) > It appears, it is used :-/ > > I have an error on tools/import_borrowers.pl: > > Software error: > Can't locate object method "columns" via package "C4::Members" at > /.../tools/import_borrowers.pl line 61. grrr... it's used without (), so my grep was not able to find it :((((
This patch has been reverted before 3.8 release because it break tools/import_borrowers, the column() function is used (revert is commit 90d13965d321d12cf2984a18c4bbcdde210526ab)
Created attachment 19285 [details] [review] re-write of the C4::Members->columns function. Greetings, Can't remember the exact circumstances I found this, but I figured if the point was to remove the MySQLism, then fixing the function is just a good solution as removing it. After all, writing specific code for the import patrons seemed like a bad idea. Many thanks to druthb for pointing me at a better solution which helped me find this one. GPML...
I figured there were multiple ways to solve this. The ideal solution: 1) doesn't change field order 2) doesn't fail on empty table 3) runs comparably well or faster Method 0: the orignal SHOW COLUMNS way --- NOT AN OPTION! --- with borrowers empty and ~100K records. Method 1: $dbh->column_info - changes field order + works for empty table - runs slower (~3-3.25ms/iteration -- empty and ~100K, 4000 iterations) Method 2: select COLUMN_NAME from information_schema.COLUMNS where TABLE_NAME='borrowers'; + keeps field order + works for empty table - runs slow (1.25ms/iteration -- empty and ~100K, 4000 iterations) Method 3: hashref - changes field order - fails on empty table - runs way slower (330ms/iteration for 100K, 100 iterations) Method 4: hashref limit 1 - changes field order - fails on empty table + fast (0.5ms/iteration for 100K, 40000 iterations) Method 5: $sth->{NAME} + keeps field order + works for empty table - runs way slower (320ms/iteration for 100K, 100 iterations) Method 6: $sth->{NAME} limit 1 + keeps field order + works for empty table + fast (0.4ms/iteration for 100K, 40000 iterations) This is why my patch uses what I call method 6 when it is the MySQL driver, and currently 5 otherwise. As new RDBMS backends become functional, I would expect elsif statements to add the equivalent method 6 SQL query for each one.
Hi Mark, I don't understand the following: + my $sql; + if ( 'mysql,' =~ /$db/ ) { + $sql = 'SELECT * FROM borrowers LIMIT 1;'; + } + else { + $sql = 'SELECT * FROM borrowers;'; + } Why don't you use the LIMIT 1 clause in all cases?
"LIMIT 1" is not ANSI SQL, though it does work for other SQL DBs, much more than the SHOW COLUMNS that was there before. I only list "mysql," in my if clause, because I figure those who wish to use a different back-end would also seek to optimize things, and thus patch the if statement. A specific non-"LIMIT 1" example is SQL Server, which uses TOP. Also, Oracle seems to use some sort of rownum() logic. So, there isn't a completely generic way of doing this in a quick fashion, but by generalizing this further and using the db_scheme2dbi in C4::Context, I believe this is a vast improvement.
Another option to throw in the mix: http://search.cpan.org/~ribasushi/DBIx-Class-0.08250/lib/DBIx/Class/ResultSource.pm#columns Once DBIX::Class support is pushed, unless it performs horribly, this is likely preferable to rolling our own.
Created attachment 19527 [details] [review] Pure ANSI SQL code to replace MySQLism I realized that a WHERE 1=0 still works for the STH method, and thus doesn't need any other checks. It will always work. :) I still have to consider the DBIx case, as gmcharlt pointed out, but this should be good in the mean time. :)
Created attachment 19534 [details] [review] Bug 7785 - MySQL-specific syntax in C4::Members columns() The initial thought was to remove this function. However, tools/import_borrowers.pl uses it. So rather than remove it to solve the problem, it was reworked to a more generic solution which runs faster. By accessing $sth->{NAME} directly, the driver becomes responsible for filling it correctly. This happens when a SELECT is done on the borrowers table. It does not even have to have data in the result set! The columns method could be more generic and used elsewhere too. Comparison between the old method and the STH method showed a difference of 35 seconds for 40k iterations versus 19 seconds for the same amount of iterations on regardless of the size of the borrowers table. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> The patch itself removes the mysql-ism and works as expected. I agree with Galen that we shouldn't be doing this ad-hoc and the introduction of DBIx would be the best solution (modulo performance issues).
QA Comment: Thanks Mark for this new patch. I found 2 (really) smalls things: 1/ please use Modern::Perl instead of warnings and strict. 2/ the sth->finish statement should be removed. Marked as Failed QA.
perlcritic -5 t/db_dependent/Members_columns.t fails when use strict; use warnings; are not used. Since all newer code that is submitted must pass perlcritic -5 testing, what should I do?
Created attachment 19575 [details] [review] Pure ANSI SQL code to replace MySQLism Corrected my default perlcriticrc file with a: ln -s ~/qa-test-tools/perlcriticrc ~/.perlcritic Made the two changes as suggested by Jonathan Druart.
Created attachment 19901 [details] [review] [SIGNED-OFF] Bug 7785 - MySQL-specific syntax in C4::Members columns() The initial thought was to remove this function. However, tools/import_borrowers.pl uses it. So rather than remove it to solve the problem, it was reworked to a more generic solution which runs faster. By accessing $sth->{NAME} directly, the driver becomes responsible for filling it correctly. This happens when a SELECT is done on the borrowers table. It does not even have to have data in the result set! The columns method could be more generic and used elsewhere too. Comparison between the old method and the STH method showed a significant time difference. The old method took 35 seconds for 40k iterations versus 19 seconds for the same amount of iterations with the STH method regardless of the size of the borrowers table. Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Looks good to me. Can be tested on tools/import_borrowers.pl?sample=1 Marked as Passed QA.
Created attachment 20274 [details] [review] Bug 7785 - MySQL-specific syntax in C4::Members columns() The initial thought was to remove this function. However, tools/import_borrowers.pl uses it. So rather than remove it to solve the problem, it was reworked to a more generic solution which runs faster. By accessing $sth->{NAME} directly, the driver becomes responsible for filling it correctly. This happens when a SELECT is done on the borrowers table. It does not even have to have data in the result set! The columns method could be more generic and used elsewhere too. Comparison between the old method and the STH method showed a significant time difference. The old method took 35 seconds for 40k iterations versus 19 seconds for the same amount of iterations with the STH method regardless of the size of the borrowers table. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Pushed to master, along with a follow-up. Thanks, Mark!