Re: [Help-smalltalk] [PATCH] dbd-sqlite: Do not fetch all rows of a select at once

[Paolo Bonzini]

Il 05/05/2013 11:24, Holger Hans Peter Freyther ha scritto:
> The current code is loading all rows of a select into the memory. For
> my application this consumes too much memory. SQLite3 does not support
> to query the size of the result set. This means that >>#rowSize can not
> determine the size of the result set. When using the >>#next/>>#atEnd
> selectors the >>#rowSize will not be available.
> 
> When using >>#rows/>>#rowSize/>>#size before any other query will result
> in all results to be fetched.

I thought this was in already! :)

Paolo

> ---
>  NEWS                               |    6 +++-
>  packages/dbd-sqlite/ChangeLog      |    8 +++++
>  packages/dbd-sqlite/ResultSet.st   |   59 
> ++++++++++++++++++++++++++----------
>  packages/dbd-sqlite/SQLiteTests.st |   39 +++++++++++++++++++++++-
>  packages/dbd-sqlite/Statement.st   |   10 ++----
>  packages/dbi/ChangeLog             |    4 +++
>  packages/dbi/ResultSet.st          |    8 +++--
>  7 files changed, 106 insertions(+), 28 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index d768a8f..0d47cf4 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,10 +1,14 @@
>  List of user-visible changes in GNU Smalltalk
>  
> -NEWS FROM 3.2.5 to 3.2.90
> +NEWS FROM 3.2.5 to 3.2.91
>  
>  o   Add PackageLoader>>#loadPackageFromFile: to load a package by using
>      a package.xml. This can make the development more effective.
>  
> +o   Change the semantic of >>#rowCount for the SQLite backend. One can
> +    either use the >>#next/#atEnd selectors to stream over the result or use
> +    the #>>rows/#rowCount selectors.
> +
>  -----------------------------------------------------------------------------
>  
>  NEWS FROM 3.2.4 to 3.2.5
> diff --git a/packages/dbd-sqlite/ChangeLog b/packages/dbd-sqlite/ChangeLog
> index 2608bb5..303158f 100644
> --- a/packages/dbd-sqlite/ChangeLog
> +++ b/packages/dbd-sqlite/ChangeLog
> @@ -1,3 +1,11 @@
> +2013-04-15  Holger Hans Peter Freyther  <address@hidden>
> +
> +     * ResultSet.st: Implement streaming usage to conserve memory.
> +     * SQLiteTests.st: Add SQLiteStreamRowTestCase class. Modify
> +     existing tests.
> +     * Statement.st: Reset the handle before executing the query
> +     and do not reset it after doing the query.
> +
>  2011-04-09  Paolo Bonzini  <address@hidden>
>  
>       * Statement.st: Move #resetAndClear inside an #ensure: block.
> diff --git a/packages/dbd-sqlite/ResultSet.st 
> b/packages/dbd-sqlite/ResultSet.st
> index 985295f..7536773 100644
> --- a/packages/dbd-sqlite/ResultSet.st
> +++ b/packages/dbd-sqlite/ResultSet.st
> @@ -8,7 +8,7 @@
>  
>  "======================================================================
>  |
> -| Copyright 2007, 2008 Free Software Foundation, Inc.
> +| Copyright 2007, 2008, 2013 Free Software Foundation, Inc.
>  | Written by Daniele Sciascia
>  |
>  | This file is part of the GNU Smalltalk class library.
> @@ -31,9 +31,12 @@
>   ======================================================================
>  "
>  
> -
>  ResultSet subclass: SQLiteResultSet [
> -    | handle rows columns index |
> +    | handle rows columns index lastRes |
> +
> +    <comment: 'This class is doing three things at once. It can handle
> +    SELECT and DML. For the result of a select a legacy fetch all interface
> +    is provided or a stream based one.'>
>                  
>      SQLiteResultSet class >> on: aStatement [
>          <category: 'instance creation'>
> @@ -46,7 +49,7 @@ ResultSet subclass: SQLiteResultSet [
>          self statement: aStatement.
>          self handle: (aStatement handle).
>          self isSelect
> -         ifTrue: [self populate]
> +         ifTrue: [lastRes := self handle exec.]
>           ifFalse: [self exec]
>      ]
>      
> @@ -58,17 +61,17 @@ ResultSet subclass: SQLiteResultSet [
>       rows := handle changes
>      ]
>  
> -    populate [
> +    populateAllRows [
>          <category: 'initialization'>
> -        | resCode |
>          
>          rows := OrderedCollection new.
> -        [ resCode := self handle exec.
> -          resCode = 100
> -        ] whileTrue: [rows addLast: 
> -                        (SQLiteRow forValues: self handle returnedRow copy 
> in: self)].
> +        [lastRes = 100]
> +            whileTrue: [
> +                rows addLast:
> +                        (SQLiteRow forValues: self handle returnedRow copy 
> in: self).
> +                lastRes := self handle exec].
>          
> -        self handle checkError: resCode = 101.
> +        self handle checkError: lastRes = 101.
>      ]
>      
>      handle [
> @@ -83,14 +86,28 @@ ResultSet subclass: SQLiteResultSet [
>  
>      next [
>          <category: 'cursor access'>
> +        | res |
> +
> +        "At the end?"
>       self atEnd ifTrue: [self error: 'No more rows'].
> -     index := index + 1.
> -     ^self rows at: index
> +
> +        "Using cached results?"
> +        rows isNil ifFalse: [index := index + 1. ^self rows at: index].
> +
> +        "first row handling.."
> +        index := index + 1.
> +        res := SQLiteRow forValues: self handle returnedRow copy in: self.
> +        lastRes := self handle exec.
> +        lastRes = 101 ifTrue: [self handle reset].
> +
> +        ^ res.
>      ]
>  
>      atEnd [
>          <category: 'cursor access'>
> -        ^index >= self rowCount
> +        ^ rows isNil
> +            ifFalse: [index >= self rows size]
> +            ifTrue:  [lastRes ~= 100].
>      ]
>      
>      position [
> @@ -100,7 +117,9 @@ ResultSet subclass: SQLiteResultSet [
>  
>      position: anInteger [
>          <category: 'stream protocol'>
> -        (anInteger between: 0 and: self size)
> +        rows isNil ifTrue: [self error: 'Can not set the position on 
> SQLite'].
> +
> +        (anInteger between: 0 and: self rows size)
>              ifTrue: [ index := anInteger ] 
>              ifFalse: [ SystemExceptions.IndexOutOfRange signalOn: self 
> withIndex: anInteger ].
>          ^index
> @@ -142,11 +161,19 @@ ResultSet subclass: SQLiteResultSet [
>      
>      rows [
>          <category: 'accessing'>
> -        ^rows
> +        rows isNil ifFalse: [^rows].
> +        index = 0 ifFalse: [
> +            ^ self error: 'Can only ask for the row set before the first 
> fetch.'].
> +
> +        self populateAllRows.
> +        ^ rows
>      ]
>  
>      rowCount [
>          <category: 'accessing'>
> +        "I'm only available for SELECT statements and only when used 
> together with
> +        >>#rows. For streaming usage with >>#atEnd and >>#next I may not be 
> used.
> +        This is because SQLite3 does not indicate the size of the query set."
>          self isSelect 
>              ifTrue: [^self rows size]
>              ifFalse: [^self error: 'Not a SELECT statement.']
> diff --git a/packages/dbd-sqlite/SQLiteTests.st 
> b/packages/dbd-sqlite/SQLiteTests.st
> index fe9d0ef..d3bb1c2 100644
> --- a/packages/dbd-sqlite/SQLiteTests.st
> +++ b/packages/dbd-sqlite/SQLiteTests.st
> @@ -120,7 +120,13 @@ SQLiteBaseTest subclass: SQLiteResultSetTestCase [
>      ]
>      
>      testRowCount [
> -        self should: [rs rowCount = 3]
> +        self should: [rs rowCount = 3].
> +        self should: [rs rows size = 3].
> +    ]
> +
> +    testMixRowCountAtEnd [
> +        rs next.
> +        self should: [rs rowCount] raise: Error description: 'May not mix 
> next/rowCount'.
>      ]
>  ]
>  
> @@ -146,6 +152,34 @@ SQLiteBaseTest subclass: SQLiteRowTestCase [
>      ]
>  ]
>  
> +SQLiteBaseTest subclass: SQLiteStreamRowTestCase [
> +    | rs |
> +
> +    setUp [
> +        super setUp.
> +        rs := self connection select: 'select * from test'.
> +    ]
> +
> +    testRead [
> +        | row |
> +        self shouldnt: [rs atEnd].
> +
> +        "First row"
> +        row := rs next.
> +        self should: [(row at: 'string_field') = 'one'].
> +        self shouldnt: [rs atEnd].
> +
> +        "Second row"
> +        row := rs next.
> +        self should: [(row at: 'string_field') = 'two'].
> +        self shouldnt: [rs atEnd].
> +
> +        "Third row"
> +        row := rs next.
> +        self should: [(row at: 'string_field') = 'three'].
> +        self should: [rs atEnd].
> +    ]
> +]
>  
>  SQLiteBaseTest subclass: SQLitePreparedStatementTestCase [
>      | stmt stmt2 stmt3 |
> @@ -204,10 +238,13 @@ TestSuite subclass: SQLiteTestSuite [
>          self addTest: (SQLiteResultSetTestCase selector: #testAtEnd).
>          self addTest: (SQLiteResultSetTestCase selector: #testColumnNames).
>          self addTest: (SQLiteResultSetTestCase selector: #testRowCount).
> +        self addTest: (SQLiteResultSetTestCase selector: 
> #testMixRowCountAtEnd).
>  
>          self addTest: (SQLiteRowTestCase selector: #testAt).
>          self addTest: (SQLiteRowTestCase selector: #testAtIndex).
>  
> +        self addTest: (SQLiteStreamRowTestCase selector: #testRead).
> +
>          self addTest: (SQLiteDMLResultSetTestCase selector: 
> #testRowsAffected).
>          
>          self addTest: (SQLitePreparedStatementTestCase selector: 
> #testExecute).
> diff --git a/packages/dbd-sqlite/Statement.st 
> b/packages/dbd-sqlite/Statement.st
> index ea6166d..9b0cfc4 100644
> --- a/packages/dbd-sqlite/Statement.st
> +++ b/packages/dbd-sqlite/Statement.st
> @@ -71,23 +71,19 @@ Statement subclass: SQLiteStatement [
>      
>      execute [
>          <category: 'querying'>
> +        self handle reset.
>          ^SQLiteResultSet on: self
>      ]
>      
>      executeWithAll: aParams [
>          <category: 'querying'>
>          | resCode |
> +        self handle reset.
>          ^[aParams keysAndValuesDo: [:i :param | 
>                  resCode := self handle bindingAt: i put: param.
>                  self handle checkError: resCode = 0].
>                                            
> -        SQLiteResultSet on: self] ensure: [self resetAndClear]
> -    ]
> -    
> -    resetAndClear [
> -        <category: 'private'>
> -        self handle reset.
> -        self handle clearBindings.
> +        SQLiteResultSet on: self] ensure: [self handle clearBindings]
>      ]
>      
>      getCommand [
> diff --git a/packages/dbi/ChangeLog b/packages/dbi/ChangeLog
> index 67b1647..e0a8c96 100644
> --- a/packages/dbi/ChangeLog
> +++ b/packages/dbi/ChangeLog
> @@ -1,3 +1,7 @@
> +2013-04-15  Holger Hans Peter Freyther  <address@hidden>
> +
> +     * ResultSet.st: Add documentation to >>#rowCount and >>#size.
> +
>  2011-04-08  Holger Hans Peter Freyther  <address@hidden>
>  
>       * Statement.st: Add Statement class>>#getCommand:.
> diff --git a/packages/dbi/ResultSet.st b/packages/dbi/ResultSet.st
> index 308a268..c3c312d 100644
> --- a/packages/dbi/ResultSet.st
> +++ b/packages/dbi/ResultSet.st
> @@ -8,7 +8,7 @@
>  "======================================================================
>  |
>  | Copyright 2006 Mike Anderson
> -| Copyright 2007, 2008, 2009 Free Software Foundation, Inc.
> +| Copyright 2007, 2008, 2009, 2013 Free Software Foundation, Inc.
>  |
>  | Written by Mike Anderson
>  |
> @@ -123,14 +123,16 @@ case I only hold the number of rows affected.'>
>      ]
>  
>      size [
> -     "Returns the number of rows in the result set."
> +     "Returns the number of rows in the result set. See >>#rowCount for
> +      details."
>       <category: 'stream protocol'>
>       ^self rowCount
>      ]
>  
>      rowCount [
>       "Returns the number of rows in the result set;
> -      error for DML statements."
> +      error for DML statements. Not all implementations allow to query
> +      the size of the ResultSet. In this case an Error will be raised."
>  
>       <category: 'accessing'>
>       self error: 'Not a SELECT statement.'
>