http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Marc Balmer <marc@msys.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|7365 | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 --- Comment #1 from Marc Balmer <marc@msys.ch> --- Created attachment 8617 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=8617&action=edit Remove the unused columns() function, which uses a MySQLism -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #8617|0 |1 is obsolete| | --- Comment #2 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 8874 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=8874&action=edit Remove the unused columns() function, which uses a MySQLism Signed off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |paul.poulain@biblibre.com --- Comment #3 from Paul Poulain <paul.poulain@biblibre.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 --- Comment #5 from Jonathan Druart <jonathan.druart@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |7975 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |ASSIGNED --- Comment #7 from Paul Poulain <paul.poulain@biblibre.com> --- This patch has been reverted before 3.8 release because it break tools/import_borrowers, the column() function is used (revert is commit 90d13965d321d12cf2984a18c4bbcdde210526ab) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 --- Comment #11 from M. Tompsett <mtompset@hotmail.com> --- "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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19285|0 |1 is obsolete| | --- Comment #13 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 19527 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19527&action=edit 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. :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19527|0 |1 is obsolete| | --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 19534 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19534&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 --- Comment #16 from M. Tompsett <mtompset@hotmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19575|0 |1 is obsolete| | --- Comment #18 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 19901 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19901&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7785 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19901|0 |1 is obsolete| | --- Comment #20 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 20274 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20274&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.