Bug 7785 - MySQL-specific syntax in C4::Members columns()
Summary: MySQL-specific syntax in C4::Members columns()
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: M. Tompsett
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 7975
Blocks: 7365
  Show dependency treegraph
 
Reported: 2012-03-21 23:37 UTC by Ian Walls
Modified: 2014-12-07 20:02 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Remove the unused columns() function, which uses a MySQLism (739 bytes, patch)
2012-03-24 08:54 UTC, Marc Balmer
Details | Diff | Splinter Review
Remove the unused columns() function, which uses a MySQLism (807 bytes, patch)
2012-04-04 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
re-write of the C4::Members->columns function. (4.03 KB, patch)
2013-06-30 01:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Pure ANSI SQL code to replace MySQLism (3.20 KB, patch)
2013-07-10 17:59 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 7785 - MySQL-specific syntax in C4::Members columns() (3.46 KB, patch)
2013-07-10 22:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Pure ANSI SQL code to replace MySQLism (3.22 KB, patch)
2013-07-11 14:55 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7785 - MySQL-specific syntax in C4::Members columns() (3.27 KB, patch)
2013-07-24 04:35 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 7785 - MySQL-specific syntax in C4::Members columns() (3.33 KB, patch)
2013-08-12 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Walls 2012-03-21 23:37:30 UTC
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.
Comment 1 Marc Balmer 2012-03-24 08:54:54 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2012-04-04 14:47:15 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2012-04-04 16:11:54 UTC
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
Comment 5 Jonathan Druart 2012-04-06 12:06:40 UTC
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.
Comment 6 Paul Poulain 2012-04-06 12:29:47 UTC
(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 :((((
Comment 7 Paul Poulain 2012-04-19 08:25:15 UTC
This patch has been reverted before 3.8 release because it break tools/import_borrowers, the column() function is used
(revert is commit 90d13965d321d12cf2984a18c4bbcdde210526ab)
Comment 8 M. Tompsett 2013-06-30 01:39:27 UTC Comment hidden (obsolete)
Comment 9 M. Tompsett 2013-06-30 03:45:35 UTC
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.
Comment 10 Jonathan Druart 2013-07-09 14:05:55 UTC
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?
Comment 11 M. Tompsett 2013-07-10 00:13:16 UTC
"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.
Comment 12 Galen Charlton 2013-07-10 15:15:26 UTC
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.
Comment 13 M. Tompsett 2013-07-10 17:59:27 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2013-07-10 22:54:43 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-07-11 08:08:19 UTC
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.
Comment 16 M. Tompsett 2013-07-11 14:31:28 UTC
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?
Comment 17 M. Tompsett 2013-07-11 14:55:38 UTC Comment hidden (obsolete)
Comment 18 Srdjan Jankovic 2013-07-24 04:35:33 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2013-08-12 13:45:42 UTC
Looks good to me. Can be tested on tools/import_borrowers.pl?sample=1

Marked as Passed QA.
Comment 20 Jonathan Druart 2013-08-12 13:46:47 UTC
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>
Comment 21 Galen Charlton 2013-08-13 16:18:08 UTC
Pushed to master, along with a follow-up.  Thanks, Mark!