Bug 28306

Summary: Allow to query database with minimal memory footprint
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Architecture, internals, and plumbingAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: dcook, jonathan.druart, kyle, m.de.rooy, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27267
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This provides a new method Koha::Database::dbh which returns a database handler without loading unnecessary stuff. This will be useful to reduce memory usage of daemons that need to check the database periodically.
Version(s) released in:
21.11.00
Bug Depends on: 28276    
Bug Blocks: 28410    
Attachments: Bug 28306: Allow to query database with minimal memory footprint
Bug 28306: Allow to query database with minimal memory footprint
Bug 28306: Allow to query database with minimal memory footprint
Bug 28306: Allow to query database with minimal memory footprint
Bug 28306: Switch back to using 'mock' instead of 'redefine'
Bug 28306: Fix POD
Bug 28306: Fix POD
Bug 28306: Fix t/db_dependent/Koha/Objects.t
Bug 28306: Trying to understand...

Description Julian Maurice 2021-05-10 06:53:51 UTC
The goal is to be able to build a database handler (dbh) and to execute queries without loading unnecessary stuff. This will be useful to reduce memory usage of daemons that need to check the database periodically
Comment 1 Julian Maurice 2021-05-10 06:55:05 UTC
Created attachment 120752 [details] [review]
Bug 28306: Allow to query database with minimal memory footprint

The goal is to be able to build a database handler (dbh) and to execute
queries without loading unnecessary stuff. This will be useful to reduce
memory usage of daemons that need to check the database
periodically

The patch provides a new method Koha::Database::dbh which returns a
database handler without loading the DBIx::Class schema. This method is
also used by DBIx::Class, so whether you use DBI or DBIx::Class, the
same method is used to initialize the connection.

The patch also moves some code in order to avoid loading C4::Context:
- C4::Context::timezone moves to Koha::Config
- C4::Context::db_scheme2dbi moves to Koha::Database

To measure memory usage I used the following commands:

* before the patch:
perl -MKoha::Database \
    -E 'Koha::Database->schema->storage->dbh->do("select 1");' \
    -E '$|=1; say $$; sleep 2' \
    | while read pid; do ps -p $pid -o rss=; done

* after the patch:
perl -MKoha::Database \
    -E 'Koha::Database->dbh->do("select 1");' \
    -E '$|=1; say $$; sleep 2' \
    | while read pid; do ps -p $pid -o rss=; done

It will give you the RSS (Resident Set Size) of the perl process in kB

What I get:
* before the patch: between 96.9MB and 97.2MB
* after the patch: between 17.8MB and 18.2MB

Note that if a timezone is configured (either from $KOHA_CONF or
TZ environment variable), Koha will load DateTime::Timezone to check if
it's valid, and it increases RSS to 36MB

Another interesting metric is the number of modules loaded:
* before the patch:
perl -MKoha::Database \
    -E 'Koha::Database->schema->storage->dbh;' \
    -E 'say scalar keys %INC'

Result: 567

* after the patch:
perl -MKoha::Database \
    -E 'Koha::Database->dbh;' \
    -E 'say scalar keys %INC'

Result: 51

Test plan:
1. Apply the patch & restart starman
2. Make sure Koha is still ok (ie. can access the database, does not
have encoding issues, ...)
3. Run the tests in t/Context.t, t/Koha/Config.t,
t/db_dependent/Koha/Database.t, t/timezones.t
Comment 2 Martin Renvoize 2021-05-10 08:04:38 UTC
This looks very interesting, I'm loving how much code cleaning goes on and the results in my testing so far are very promising.. I'll continue to play a little and then SO, great work Julian
Comment 3 Martin Renvoize 2021-05-10 14:11:34 UTC
Created attachment 120794 [details] [review]
Bug 28306: Allow to query database with minimal memory footprint

The goal is to be able to build a database handler (dbh) and to execute
queries without loading unnecessary stuff. This will be useful to reduce
memory usage of daemons that need to check the database
periodically

The patch provides a new method Koha::Database::dbh which returns a
database handler without loading the DBIx::Class schema. This method is
also used by DBIx::Class, so whether you use DBI or DBIx::Class, the
same method is used to initialize the connection.

The patch also moves some code in order to avoid loading C4::Context:
- C4::Context::timezone moves to Koha::Config
- C4::Context::db_scheme2dbi moves to Koha::Database

To measure memory usage I used the following commands:

* before the patch:
perl -MKoha::Database \
    -E 'Koha::Database->schema->storage->dbh->do("select 1");' \
    -E '$|=1; say $$; sleep 2' \
    | while read pid; do ps -p $pid -o rss=; done

* after the patch:
perl -MKoha::Database \
    -E 'Koha::Database->dbh->do("select 1");' \
    -E '$|=1; say $$; sleep 2' \
    | while read pid; do ps -p $pid -o rss=; done

It will give you the RSS (Resident Set Size) of the perl process in kB

What I get:
* before the patch: between 96.9MB and 97.2MB
* after the patch: between 17.8MB and 18.2MB

Note that if a timezone is configured (either from $KOHA_CONF or
TZ environment variable), Koha will load DateTime::Timezone to check if
it's valid, and it increases RSS to 36MB

Another interesting metric is the number of modules loaded:
* before the patch:
perl -MKoha::Database \
    -E 'Koha::Database->schema->storage->dbh;' \
    -E 'say scalar keys %INC'

Result: 567

* after the patch:
perl -MKoha::Database \
    -E 'Koha::Database->dbh;' \
    -E 'say scalar keys %INC'

Result: 51

Test plan:
1. Apply the patch & restart starman
2. Make sure Koha is still ok (ie. can access the database, does not
have encoding issues, ...)
3. Run the tests in t/Context.t, t/Koha/Config.t,
t/db_dependent/Koha/Database.t, t/timezones.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2021-05-10 14:12:17 UTC
All works nicely and as expected.. I think this could lead to some real improvements in some of our regular command-line scripts etc.

Signing off
Comment 5 David Cook 2021-05-10 23:56:52 UTC
Sounds good to me. 

I would've liked this as a patch set to make it easier to read. It looks like there are quite a few different changes here, so I think QA will need to take time to unpick them to make sure everything is OK. Unfortunately, I don't have that time right now. 

But +1 to the idea.
Comment 6 Nick Clemens 2021-07-23 16:31:43 UTC
Created attachment 123145 [details] [review]
Bug 28306: Allow to query database with minimal memory footprint

The goal is to be able to build a database handler (dbh) and to execute
queries without loading unnecessary stuff. This will be useful to reduce
memory usage of daemons that need to check the database
periodically

The patch provides a new method Koha::Database::dbh which returns a
database handler without loading the DBIx::Class schema. This method is
also used by DBIx::Class, so whether you use DBI or DBIx::Class, the
same method is used to initialize the connection.

The patch also moves some code in order to avoid loading C4::Context:
- C4::Context::timezone moves to Koha::Config
- C4::Context::db_scheme2dbi moves to Koha::Database

To measure memory usage I used the following commands:

* before the patch:
perl -MKoha::Database \
    -E 'Koha::Database->schema->storage->dbh->do("select 1");' \
    -E '$|=1; say $$; sleep 2' \
    | while read pid; do ps -p $pid -o rss=; done

* after the patch:
perl -MKoha::Database \
    -E 'Koha::Database->dbh->do("select 1");' \
    -E '$|=1; say $$; sleep 2' \
    | while read pid; do ps -p $pid -o rss=; done

It will give you the RSS (Resident Set Size) of the perl process in kB

What I get:
* before the patch: between 96.9MB and 97.2MB
* after the patch: between 17.8MB and 18.2MB

Note that if a timezone is configured (either from $KOHA_CONF or
TZ environment variable), Koha will load DateTime::Timezone to check if
it's valid, and it increases RSS to 36MB

Another interesting metric is the number of modules loaded:
* before the patch:
perl -MKoha::Database \
    -E 'Koha::Database->schema->storage->dbh;' \
    -E 'say scalar keys %INC'

Result: 567

* after the patch:
perl -MKoha::Database \
    -E 'Koha::Database->dbh;' \
    -E 'say scalar keys %INC'

Result: 51

Test plan:
1. Apply the patch & restart starman
2. Make sure Koha is still ok (ie. can access the database, does not
have encoding issues, ...)
3. Run the tests in t/Context.t, t/Koha/Config.t,
t/db_dependent/Koha/Database.t, t/timezones.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens 2021-07-23 16:32:24 UTC
Adding my SO - there are POD complaints from QA tools - this has potential to break things so another pair of eyes for QA would be nice
Comment 8 Kyle M Hall 2021-07-23 19:10:27 UTC
0.11

t/timezones.t .. Can't locate object method "redefine" via package "Test::MockModule" at /kohadevbox/koha/t/lib/Mocks.pm line 56, <DATA> line 1.

koha-testing-docker has Test::MockModule v0.11, but the method "redefine" was not added until v0.13
Comment 9 Tomás Cohen Arazi 2021-07-23 19:13:29 UTC
(In reply to Kyle M Hall from comment #8)
> 0.11
> 
> t/timezones.t .. Can't locate object method "redefine" via package
> "Test::MockModule" at /kohadevbox/koha/t/lib/Mocks.pm line 56, <DATA> line 1.
> 
> koha-testing-docker has Test::MockModule v0.11, but the method "redefine"
> was not added until v0.13

I ran into that recently. It is a pity, but we should use ->mock until all our supported distros ship the newer Test::MockModule.
Comment 10 Kyle M Hall 2021-07-23 19:22:44 UTC
Created attachment 123148 [details] [review]
Bug 28306: Allow to query database with minimal memory footprint

The goal is to be able to build a database handler (dbh) and to execute
queries without loading unnecessary stuff. This will be useful to reduce
memory usage of daemons that need to check the database
periodically

The patch provides a new method Koha::Database::dbh which returns a
database handler without loading the DBIx::Class schema. This method is
also used by DBIx::Class, so whether you use DBI or DBIx::Class, the
same method is used to initialize the connection.

The patch also moves some code in order to avoid loading C4::Context:
- C4::Context::timezone moves to Koha::Config
- C4::Context::db_scheme2dbi moves to Koha::Database

To measure memory usage I used the following commands:

* before the patch:
perl -MKoha::Database \
    -E 'Koha::Database->schema->storage->dbh->do("select 1");' \
    -E '$|=1; say $$; sleep 2' \
    | while read pid; do ps -p $pid -o rss=; done

* after the patch:
perl -MKoha::Database \
    -E 'Koha::Database->dbh->do("select 1");' \
    -E '$|=1; say $$; sleep 2' \
    | while read pid; do ps -p $pid -o rss=; done

It will give you the RSS (Resident Set Size) of the perl process in kB

What I get:
* before the patch: between 96.9MB and 97.2MB
* after the patch: between 17.8MB and 18.2MB

Note that if a timezone is configured (either from $KOHA_CONF or
TZ environment variable), Koha will load DateTime::Timezone to check if
it's valid, and it increases RSS to 36MB

Another interesting metric is the number of modules loaded:
* before the patch:
perl -MKoha::Database \
    -E 'Koha::Database->schema->storage->dbh;' \
    -E 'say scalar keys %INC'

Result: 567

* after the patch:
perl -MKoha::Database \
    -E 'Koha::Database->dbh;' \
    -E 'say scalar keys %INC'

Result: 51

Test plan:
1. Apply the patch & restart starman
2. Make sure Koha is still ok (ie. can access the database, does not
have encoding issues, ...)
3. Run the tests in t/Context.t, t/Koha/Config.t,
t/db_dependent/Koha/Database.t, t/timezones.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2021-07-23 19:22:54 UTC
Created attachment 123149 [details] [review]
Bug 28306: Switch back to using 'mock' instead of 'redefine'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2021-07-23 19:22:58 UTC
Created attachment 123150 [details] [review]
Bug 28306: Fix POD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2021-07-23 19:25:01 UTC
Created attachment 123151 [details] [review]
Bug 28306: Fix POD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Jonathan Druart 2021-08-31 08:28:29 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 15 Jonathan Druart 2021-08-31 10:42:24 UTC
A very weird side-effects caused by this patch, t/db_dependent/Koha/Objects.t is failing with:

t/db_dependent/Koha/Objects.t .. 18/24                         
    #   Failed test at t/db_dependent/Koha/Objects.t line 1152.
                                                               
    #   Failed test at t/db_dependent/Koha/Objects.t line 1164.
    # Looks like you failed 2 tests of 13.                                  
                                                                            
#   Failed test 'filter_by_last_update'        
#   at t/db_dependent/Koha/Objects.t line 1172.

If you inspect the exception it contains "The method 1970-12-31->dt_from_string is not covered by tests!"

Could you have a look?
Comment 16 Jonathan Druart 2021-08-31 10:43:57 UTC
(Prefixing the dt_from_string calls in filter_by_last_update fixes the error, but I don't think it's the correct fix)
Comment 17 Julian Maurice 2021-08-31 12:06:16 UTC
Created attachment 124274 [details] [review]
Bug 28306: Fix t/db_dependent/Koha/Objects.t

It looks like a circular dependency problem where methods are
not exported correctly

There is at least one circular dependency here:

    C4::Context -> Koha::Caches -> Koha::Cache -> C4::Context
    (-> means "uses")

The tests pass if C4::Context is loaded early, so the patch does that.

Circular dependencies should be fixed ideally, but it is a little
more complicated, and I don't have the time right now...
Comment 18 Jonathan Druart 2021-08-31 13:20:46 UTC
Created attachment 124282 [details] [review]
Bug 28306: Trying to understand...

Use of uninitialized value $class in sprintf at /kohadevbox/koha/Koha/Objects.pm line 594.
The method ->dt_from_string is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Objects.pm line 594
Koha::Objects::AUTOLOAD at /kohadevbox/koha/Koha/Objects.pm line 280
Koha::Objects::filter_by_last_update('Koha::Patrons=HASH(0x55baa017c208)', 'HASH(0x55baa017c178)') called at test.pl line 5

We are hitting AUTOLOAD, however I am not expecting to reach AUTOLOAD as
I am considering dt_from_string() defined in Koha::Objects.

https://perldoc.perl.org/perlsub#Autoloading
Comment 19 Jonathan Druart 2021-09-09 09:02:29 UTC
As I didn't get additional help I've pushed Julian's patch.
Comment 20 Marcel de Rooy 2021-09-22 06:41:23 UTC
(In reply to Jonathan Druart from comment #18)
> Bug 28306: Trying to understand...

Does this commit title meet our standards? :)

What I would like to understand too, is what happens with Objects.t on this patch. At first glance looks like deleting lots of lines since we cannot resolve a problem with AUTOLOAD or so ?
Comment 21 Jonathan Druart 2021-09-22 07:35:30 UTC
(In reply to Marcel de Rooy from comment #20)
> (In reply to Jonathan Druart from comment #18)
> > Bug 28306: Trying to understand...
> 
> Does this commit title meet our standards? :)
> 
> What I would like to understand too, is what happens with Objects.t on this
> patch. At first glance looks like deleting lots of lines since we cannot
> resolve a problem with AUTOLOAD or so ?

It was not meant to be pushed.

I sent the following email to the QA team on August 31:

"""
Wanna have some fun?
I am struggling with the following situation.
Today I pushed bug 28306 and noticed a failure on Objects.t (see from
coment 15 - https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28306#c15)
It's absolutely not related with the original patches, but seems to be
linked with 17600 (the EXPORT_OK funkiness).
So there is an easy fix, two actually:
* use C4::Context from the test file (even not used directly from there...)
* Use full qualified namespace for dt_from_string
That could work, but we don't know what's going on exactly.

I've attached a patch to play (comment 18), you just need to run a 3
lines script to recreate the problem
A simple call to dt_from_string generates a "The method
->dt_from_string is not covered by tests!" error.
Because we are hitting AUTOLOAD (??). But the POD is clear, we
shouldn't https://perldoc.perl.org/perlsub#Autoloading
And, we are using dt_from_string that way from Koha::Object (singular)
and it's working perfectly.

So, what's going on? Maybe coming from Koha::DateUtils that is not
using EXPORT_OK? I tried, see bug 28931. You can apply the patch from
there and try again with the 3 lines script.

Unfortunately, it's not fixed, but maybe part of the solution...
I need more eyes one this one, please help!
"""

I didn't receive any feedback and decided to abandon the investigation and push the (meaningless) fix.
Comment 22 Marcel de Rooy 2021-09-22 11:07:23 UTC
(In reply to Jonathan Druart from comment #21)
> I sent the following email to the QA team on August 31:
> 
> I didn't receive any feedback and decided to abandon the investigation and
> push the (meaningless) fix.

Yeah I saw it but the security stuff came after it right away and has put this one in the shadow.