Skip Menu |
 

This queue is for tickets about the DBIx-SearchBuilder CPAN distribution.

Report information
The Basics
Id: 96902
Status: resolved
Priority: 0/
Queue: DBIx-SearchBuilder

People
Owner: Nobody in particular
Requestors: michelle [...] sorbs.net
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: 1 (2?) bugs in ->fields()
Date: Thu, 03 Jul 2014 04:31:11 +0200
To: bug-DBIx-SearchBuilder [...] rt.cpan.org
From: Michelle Sullivan <michelle [...] sorbs.net>
Download (untitled) / with headers
text/plain 2.5k
Ok when testing RT 4.0 with RT servers on a different network from the DB servers it's unusably slow. The problem is down to calling ->Fields() Here's a patch that makes Fields load only the fields for tables that are requested - rather than from every schema (which can be huge - someone recently told me that they have 119 schemas in the same database!) --- DBIx/SearchBuilder/Handle.pm.orig 2014-06-24 05:21:54.000000000 -0500 +++ DBIx/SearchBuilder/Handle.pm 2014-06-24 05:21:41.000000000 -0500 @@ -1429,14 +1429,16 @@ sub Fields { my $self = shift; my $table = shift; + my $lctn = lc $table; - unless ( keys %FIELDS_IN_TABLE ) { - my $sth = $self->dbh->column_info( undef, '', '%', '%' ) + unless ( keys %FIELDS_IN_TABLE && exists $FIELDS_IN_TABLE{$lctn} ) { + my $sth = $self->dbh->column_info( undef, '', $table, '%' ) or return (); my $info = $sth->fetchall_arrayref({}); foreach my $e ( @$info ) { - push @{ $FIELDS_IN_TABLE{ lc $e->{'TABLE_NAME'} } ||= [] }, lc $e->{'COLUMN_NAME'}; + push @{ $FIELDS_IN_TABLE{ $lctn } ||= [] }, lc $e->{'COLUMN_NAME'}; } + $FIELDS_IN_TABLE{ $lctn } ||= []; # empty list so we don't call column_info() again. } return @{ $FIELDS_IN_TABLE{$lctn } || [] }; Second bug (which this patch mostly exaggerates rather than fixing) is it is valid to have tables of "Table", "table", "taBLE" which are all different by using the lowercase table name in the hash only the last table will be recorded, and worse any application wanting fields on 'Table' where 'taBLE' was stored would get the wrong fields. The patch will exaggerate the bug because it will store the case-sensitive table match (or no match) in the hash, therefore if the ->Fields() call is made with the wrong case (not matching any table) no results will be found... On the other hand it can make it better because if the earlier schema is required (ie: "Table" instead of "taBLE") this patch will return the correct fields rather than the wrong fields. Performance increase for running RT4 with just 3 schemas on PostgreSQL is a single ticket load will go from 4-6 minutes (without the patch) to 7 seconds (with the patch) - the RT front end to DB server round trip time was just 20ms during this testing. The response on the RT Users mailing list was a less than helpful "it's not designed to run on a different network" and no further responses even when submitting the patch suggestion. Regards, -- Michelle Sullivan http://www.mhix.org/
Subject: Re: [rt.cpan.org #96902] AutoReply: 1 (2?) bugs in ->fields()
Date: Thu, 03 Jul 2014 04:55:14 +0200
To: bug-DBIx-SearchBuilder [...] rt.cpan.org
From: Michelle Sullivan <michelle [...] sorbs.net>
Download (untitled) / with headers
text/plain 167b
Addendum: MySQL users will not see the second bug as MySQL table names are not case sensitive like PostgreSQL and others. -- Michelle Sullivan http://www.mhix.org/
Subject: Re: [rt.cpan.org #96902] 1 (2?) bugs in ->fields()
Date: Tue, 08 Jul 2014 15:37:44 -0400
To: bug-DBIx-SearchBuilder [...] rt.cpan.org
From: Alex Vandiver <alexmv [...] bestpractical.com>
Download (untitled) / with headers
text/plain 1.8k
On 07/02/2014 10:31 PM, Michelle Sullivan via RT wrote: Show quoted text
> Ok when testing RT 4.0 with RT servers on a different network from the > DB servers it's unusably slow. > > The problem is down to calling ->Fields() > > Here's a patch that makes Fields load only the fields for tables that > are requested - rather than from every schema (which can be huge - > someone recently told me that they have 119 schemas in the same database!)
Thanks for the patch. I've applied it to master, and DBIx-SearchBuilder 1.65_01 is on its way to CPAN with it. Show quoted text
> Second bug (which this patch mostly exaggerates rather than fixing) is > it is valid to have tables of "Table", "table", "taBLE" which are all > different by using the lowercase table name in the hash only the last > table will be recorded, and worse any application wanting fields on > 'Table' where 'taBLE' was stored would get the wrong fields.
Noted in the commit message. I would be surprised if DBIx::SearchBuilder did not make that assumption already -- and it is certainly asking for confusion, if nothing else. :) Show quoted text
> Performance increase for running RT4 with just 3 schemas on PostgreSQL > is a single ticket load will go from 4-6 minutes (without the patch) to > 7 seconds (with the patch) - the RT front end to DB server round trip > time was just 20ms during this testing.
Local testing (with a pg host 20ms away) shows the first page load time drops from 130s to 7s. Subsequent page loads are under 3s; if you are still seeing slowness after the first query to each process, it implies that your mod_perl children are never being reused -- which may yield additional speedups if fixed. Show quoted text
> The response on the RT Users > mailing list was a less than helpful "it's not designed to run on a > different network" and no further responses even when submitting the > patch suggestion.
Apologies -- I had your email earmarked to respond, but hadn't had time to do so. - Alex
Subject: Re: [rt.cpan.org #96902] 1 (2?) bugs in ->fields()
Date: Sun, 13 Jul 2014 14:14:22 +0200
To: bug-DBIx-SearchBuilder [...] rt.cpan.org, Alex Vandiver <alexmv [...] bestpractical.com>
From: Michelle Sullivan <michelle [...] sorbs.net>
Download (untitled) / with headers
text/plain 6.9k
Alex Vandiver via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=96902 > > > On 07/02/2014 10:31 PM, Michelle Sullivan via RT wrote: >
>> Ok when testing RT 4.0 with RT servers on a different network from the >> DB servers it's unusably slow. >> >> The problem is down to calling ->Fields() >> >> Here's a patch that makes Fields load only the fields for tables that >> are requested - rather than from every schema (which can be huge - >> someone recently told me that they have 119 schemas in the same database!) >>
> > Thanks for the patch. I've applied it to master, and DBIx-SearchBuilder > 1.65_01 is on its way to CPAN with it. >
I can't actually see it in the source - though I can see the revision bump. You know I have been thinking about the patch (and the docs) ... There is a general problem we are missing... DBIx::SearchBuilder was originally build by Jesse, and originally against MySQL to make things easier for people (particularly those that didn't know what they were doing) No I don't know how many DBs are case insensitive, but I suspect it's very few (notable: MySQL and derivatives.) However, the whole function is written to lowercase everything (indicating case insensitivity is the expected 'norm'.. which isn't the case.) I wrote a second patch for it for the FreeBSD ports (here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=191734 ) which removed all case-sensitivity from it... I think that would actually be the correct way to go as a 'half way house' that it's in with my initial patch could cause more confusion and certainly will result in more people using it in an incorrect way. Thoughts? Here is my 'complete fix' patch that removes the case sensitivity from DBIx::SearchBuilder->Fields (actually: DBIx::SearchBuilder::Handle->Fields()) and an update to the documentation. diff -Nru DBIx-SearchBuilder-1.65.orig/lib/DBIx/SearchBuilder/Handle.pm DBIx-SearchBuilder-1.65/lib/DBIx/SearchBuilder/Handle.pm --- DBIx-SearchBuilder-1.65.orig/lib/DBIx/SearchBuilder/Handle.pm 2014-07-13 06:51:05.000000000 -0500 +++ DBIx-SearchBuilder-1.65/lib/DBIx/SearchBuilder/Handle.pm 2014-07-13 06:42:11.000000000 -0500 @@ -1430,16 +1430,16 @@ my $self = shift; my $table = shift; - unless ( keys %FIELDS_IN_TABLE ) { - my $sth = $self->dbh->column_info( undef, '', '%', '%' ) + unless ( keys %FIELDS_IN_TABLE && exists $FIELDS_IN_TABLE{$table} ) { + my $sth = $self->dbh->column_info( undef, '', $table, '%' ) or return (); my $info = $sth->fetchall_arrayref({}); foreach my $e ( @$info ) { - push @{ $FIELDS_IN_TABLE{ lc $e->{'TABLE_NAME'} } ||= [] }, lc $e->{'COLUMN_NAME'}; + push @{ $FIELDS_IN_TABLE{ $e->{'TABLE_NAME'} } ||= [] }, $e->{'COLUMN_NAME'}; } } - return @{ $FIELDS_IN_TABLE{ lc $table } || [] }; + return @{ $FIELDS_IN_TABLE{ $table } || [] }; } diff -Nru DBIx-SearchBuilder-1.65.orig/lib/DBIx/SearchBuilder.pm DBIx-SearchBuilder-1.65/lib/DBIx/SearchBuilder.pm --- DBIx-SearchBuilder-1.65.orig/lib/DBIx/SearchBuilder.pm 2014-07-13 06:51:05.000000000 -0500 +++ DBIx-SearchBuilder-1.65/lib/DBIx/SearchBuilder.pm 2014-07-13 06:50:28.000000000 -0500 @@ -1785,9 +1785,13 @@ =head2 Fields TABLE -Return a list of fields in TABLE, lowercased. +Return a list of fields in TABLE -TODO: Why are they lowercased? +Note: Previous versions returned all column names as lowercase, this +call is now case sensitive in both the table name and the column names +returned. If your database is not case sensitive (eg: MySQL) this +will have no effect. If your database is case sensitive (eg: Oracle and +PostgreSQL) you have to use the correct Case at all times! =cut @@ -1801,6 +1805,8 @@ Returns true if TABLE has field FIELD. Return false otherwise +Note: Both TABLE and FIELD are case-sensitive (See: L</Fields>) + =cut sub HasField { Show quoted text
>
>> Second bug (which this patch mostly exaggerates rather than fixing) is >> it is valid to have tables of "Table", "table", "taBLE" which are all >> different by using the lowercase table name in the hash only the last >> table will be recorded, and worse any application wanting fields on >> 'Table' where 'taBLE' was stored would get the wrong fields. >>
> > Noted in the commit message. I would be surprised if > DBIx::SearchBuilder did not make that assumption already -- and it is > certainly asking for confusion, if nothing else. :) >
There was actually a knock-on effect... Before the patch if you called 'HasField' with *non* lowercased table and *non* lowercased field (column) names it would always return FALSE (this was undocumented.) Eg before patching (using PostgreSQL) these will all return FALSE: CREATE TABLE "Table1" ("Column1" BIGINT DEFAULT NOT NULL); DBIx::SearchBuilder->HasField(TABLE => "Table1", FIELD => "column1"); DBIx::SearchBuilder->HasField(TABLE => "table1", FIELD => "column1"); DBIx::SearchBuilder->HasField(TABLE => "Table1", FIELD => "Column1"); This, however, would return true: DBIx::SearchBuilder->HasField(TABLE => "table1", FIELD => "Column1"); (This is because Fields() cached everything lowercase, but HasFields() did not lowercase the input parameters.) Show quoted text
>
>> Performance increase for running RT4 with just 3 schemas on PostgreSQL >> is a single ticket load will go from 4-6 minutes (without the patch) to >> 7 seconds (with the patch) - the RT front end to DB server round trip >> time was just 20ms during this testing. >>
> > Local testing (with a pg host 20ms away) shows the first page load time > drops from 130s to 7s. Subsequent page loads are under 3s; if you are > still seeing slowness after the first query to each process, it implies > that your mod_perl children are never being reused -- which may yield > additional speedups if fixed. >
This seems correct - however I have followed all the docs and used my knowledge and can't find the issue (SORBS uses ModPerl2 extensively, so I know how I *should* do it - however I am also not using Mason, or many of the public modules, so it could be I missed something.) Show quoted text
>
>> The response on the RT Users >> mailing list was a less than helpful "it's not designed to run on a >> different network" and no further responses even when submitting the >> patch suggestion. >>
> > Apologies -- I had your email earmarked to respond, but hadn't had time > to do so. >
No problem, and am sorry, I (like many) want answers immediately, however unlike most I do know what it's like (especially running SORBS) so I give people a week or 2 and if I haven't even got an ack I treat as "They missed my post" - resend - then a week later "Ok, now I'm just being ignored" and this has been frustrating me a lot, I have a chance on making all public support for $employer (redacted as this post will be in the public space) into RT, and if I don't get this all fixed and working, it'll all go to SalesForce (which shows they are looking at paying for something...! ie not just another Freebie) Regards, Michelle -- Michelle Sullivan http://www.mhix.org/
Subject: Re: [rt.cpan.org #96902] 1 (2?) bugs in ->fields()
Date: Tue, 19 Aug 2014 18:10:51 -0400
To: bug-DBIx-SearchBuilder [...] rt.cpan.org
From: Alex Vandiver <alexmv [...] bestpractical.com>
Download (untitled) / with headers
text/plain 283b
On 07/13/2014 08:14 AM, Michelle Sullivan wrote: Show quoted text
> Here is my 'complete fix' patch that removes the case sensitivity from > DBIx::SearchBuilder->Fields (actually: > DBIx::SearchBuilder::Handle->Fields()) and an update to the documentation.
Applied and released in 1.65_02. - Alex


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.