i#102625 avoid fetching same row twice in different queries

We do a "SELECT * FROM table" just to fetch the primary key columns;
so reuse the same XResultSet to fetch all columns.
Else, we immediately issue a "SELECT * FROM table WHERE
primary_key=current_value" to read the other columns, which is
wasteful and particularly silly.

Commit 1ae17f5b03 already tried
to do that, but was essentially reverted piecewise because
it caused fdo#47520, fdo#48345, fdo#50372.

Commit c08067d6da94743d53217cbc26cffae00a22dc3a thought it did that,
but actually reverted commit 1ae17f5b03.

This implementation fetches the whole current row and caches it in memory;
only one row is cached: when the current row changes, the cache contains
the new current row.

This could be problematic (wrt to memory consumption) if the current
row is big (e.g. with BLOBs) and nobody is interested in the data
anyway (as would often be the case with BLOBs). Note that because of
our "SELECT *", the driver most probably has it in memory already
anyway, so we don't make the situation that much worse.

This could be incrementally improved with a heuristic of not
preemptively caching binary data (and also not LONGVARCHAR / TEXT /
MEMO / ...); a getFOO on these columns would issue a specific "SELECT
column FROM table WHERE primary_key=current_value" each time.

The *real* complete fix to all these issues would be to not do "SELECT
*" at all. Use "SELECT pkey_col1, pkey_col2, ..." when we are only
interested in the key columns. As to data, somehow figure out which
columns were ar interested in and "SELECT" only these (and maybe only
those with "small datatype"?). Interesting columns could be determined
by our caller (creator) as an argument to our constructor, or some
heuristic (no binary data, no "big" unbound data).
Also be extra smart and use *(m_aKeyIter) when getFOO is called
on a column included in it (and don't include it in any subsequent
SELECT).

However, there are several pitfalls.

One is buggy drivers that give use column names of columns that we
cannot fetch :-| Using "SELECT *" works around that because the driver
there *obviously* gives us only fetchable columns in the result.

Another one is the very restrictive nature of some database access
technologies. Take for example ODBC:

 - Data can be fetched only *once* (with the SQLGetData interface;
   bound columns offer a way around that, but that's viable only for
   constant-length data, not variable-length data).

   This could be addressed by an intelligent & lazy cache.

 - Data must be fetched in increasing order of column number
   (again, this is about SQLGetData).

   This is a harder issue. The current solution has the nice advantage
   of completely isolating the rest of LibO from these restrictions.

   I don't currently see how to cleanly avoid (potentially
   unnecessarily) caching column 4 if we are asked for column 3 then
   column 5, just in case we are asked for column 4 later on, unless
   we issue a specific "SELECT column4" later. But the latter would be
   quite expensive in terms of app-to-database roudtripe times :-( and
   thus creates another performance issue.

Change-Id: I999b3f8f0b8a215acb390ffefc839235346e8353

dbaccess/source/core/api/KeySet.cxx

@@ -378,6 +378,12 @@ void OKeySet::executeStatement(::rtl::OUStringBuffer& io_aFilter,const ::rtl::OU
     ::comphelper::disposeComponent(io_xAnalyzer);
 }
 
+void OKeySet::invalidateRow()
+{
+    m_xRow = NULL;
+    ::comphelper::disposeComponent(m_xSet);
+}
+
 Any SAL_CALL OKeySet::getBookmark() throw(SQLException, RuntimeException)
 {
     RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen@sun.com", "OKeySet::getBookmark" );
@@ -403,10 +409,11 @@ sal_Bool SAL_CALL OKeySet::moveRelativeToBookmark( const Any& bookmark, sal_Int3
     m_aKeyIter = m_aKeyMap.find(::comphelper::getINT32(bookmark));
     if(m_aKeyIter != m_aKeyMap.end())
     {
-        relative(rows);
+        return relative(rows);
     }
 
-    return !isBeforeFirst() && !isAfterLast();
+    invalidateRow();
+    return false;
 }
 
 sal_Int32 SAL_CALL OKeySet::compareBookmarks( const Any& _first, const Any& _second ) throw(SQLException, RuntimeException)
@@ -1119,7 +1126,12 @@ sal_Bool SAL_CALL OKeySet::next(  ) throw(SQLException, RuntimeException)
     {
         // not yet all records fetched, but we reached the end of those we fetched
         // try to fetch one more row
-        if (!fetchRow())
+        if (fetchRow())
+        {
+            OSL_ENSURE(!isAfterLast(), "fetchRow succeeded, but isAfterLast()");
+            return true;
+        }
+        else
         {
             // nope, we arrived at end of data
             m_aKeyIter = m_aKeyMap.end();
@@ -1167,8 +1179,7 @@ void SAL_CALL OKeySet::beforeFirst(  ) throw(SQLException, RuntimeException)
     RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen@sun.com", "OKeySet::beforeFirst" );
     m_bInserted = m_bUpdated = m_bDeleted = sal_False;
     m_aKeyIter = m_aKeyMap.begin();
-    m_xRow = NULL;
-    ::comphelper::disposeComponent(m_xSet);
+    invalidateRow();
 }
 
 void SAL_CALL OKeySet::afterLast(  ) throw(SQLException, RuntimeException)
@@ -1177,8 +1188,7 @@ void SAL_CALL OKeySet::afterLast(  ) throw(SQLException, RuntimeException)
     m_bInserted = m_bUpdated = m_bDeleted = sal_False;
     fillAllRows();
     m_aKeyIter = m_aKeyMap.end();
-    m_xRow = NULL;
-    ::comphelper::disposeComponent(m_xSet);
+    invalidateRow();
 }
 
 sal_Bool SAL_CALL OKeySet::first(  ) throw(SQLException, RuntimeException)
@@ -1187,8 +1197,14 @@ sal_Bool SAL_CALL OKeySet::first(  ) throw(SQLException, RuntimeException)
     m_bInserted = m_bUpdated = m_bDeleted = sal_False;
     m_aKeyIter = m_aKeyMap.begin();
     ++m_aKeyIter;
-    if(m_aKeyIter == m_aKeyMap.end() && !fetchRow())
-        m_aKeyIter = m_aKeyMap.end();
+    if(m_aKeyIter == m_aKeyMap.end())
+    {
+        if (!fetchRow())
+        {
+            m_aKeyIter = m_aKeyMap.end();
+            return false;
+        }
+    }
     else
         refreshRow();
     return m_aKeyIter != m_aKeyMap.end() && m_aKeyIter != m_aKeyMap.begin();
@@ -1203,12 +1219,17 @@ sal_Bool OKeySet::last_checked( sal_Bool i_bFetchRow)
 {
     RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen@sun.com", "OKeySet::last_checked" );
     m_bInserted = m_bUpdated = m_bDeleted = sal_False;
-    fillAllRows();
+    bool fetchedRow = fillAllRows();
 
     m_aKeyIter = m_aKeyMap.end();
     --m_aKeyIter;
-    if ( i_bFetchRow )
-        refreshRow();
+    if ( !fetchedRow )
+    {
+        if ( i_bFetchRow )
+            refreshRow();
+        else
+            invalidateRow();
+    }
     return m_aKeyIter != m_aKeyMap.end() && m_aKeyIter != m_aKeyMap.begin();
 }
 
@@ -1230,10 +1251,11 @@ sal_Bool OKeySet::absolute_checked( sal_Int32 row,sal_Bool i_bFetchRow )
     RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen@sun.com", "OKeySet::absolute" );
     m_bInserted = m_bUpdated = m_bDeleted = sal_False;
     OSL_ENSURE(row,"absolute(0) isn't allowed!");
+    bool fetchedRow = false;
     if(row < 0)
     {
         if(!m_bRowCountFinal)
-            fillAllRows();
+            fetchedRow = fillAllRows();
 
         for(;row < 0 && m_aKeyIter != m_aKeyMap.begin();++row)
             --m_aKeyIter;
@@ -1242,18 +1264,32 @@ sal_Bool OKeySet::absolute_checked( sal_Int32 row,sal_Bool i_bFetchRow )
     {
         if(row >= (sal_Int32)m_aKeyMap.size())
         {
+            // we don't have this row
             if(!m_bRowCountFinal)
             {
+                // but there may still be rows to fetch.
                 sal_Bool bNext = sal_True;
                 for(sal_Int32 i=m_aKeyMap.size()-1;i < row && bNext;++i)
                     bNext = fetchRow();
+                // it is guaranteed that the above loop has executed at least once,
+                // that is fetchRow called at least once.
                 if ( bNext )
                 {
-                    i_bFetchRow = true;
+                    fetchedRow = true;
+                }
+                else
+                {
+                    // reached end of data before desired row
+                    m_aKeyIter = m_aKeyMap.end();
+                    return false;
                 }
             }
             else
+            {
+                // no more rows to fetch -> fail
                 m_aKeyIter = m_aKeyMap.end();
+                return false;
+            }
         }
         else
         {
@@ -1262,8 +1298,13 @@ sal_Bool OKeySet::absolute_checked( sal_Int32 row,sal_Bool i_bFetchRow )
                 ++m_aKeyIter;
         }
     }
-    if ( i_bFetchRow )
-        refreshRow();
+    if ( !fetchedRow )
+    {
+        if ( i_bFetchRow )
+            refreshRow();
+        else
+            invalidateRow();
+    }
 
     return m_aKeyIter != m_aKeyMap.end() && m_aKeyIter != m_aKeyMap.begin();
 }
@@ -1288,6 +1329,8 @@ sal_Bool OKeySet::previous_checked( sal_Bool i_bFetchRow )
         --m_aKeyIter;
         if ( i_bFetchRow )
             refreshRow();
+        else
+            invalidateRow();
     }
     return m_aKeyIter != m_aKeyMap.begin();
 }
@@ -1344,8 +1387,7 @@ void SAL_CALL OKeySet::refreshRow() throw(SQLException, RuntimeException)
 {
     RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen@sun.com", "OKeySet::refreshRow" );
 
-    m_xRow = NULL;
-    ::comphelper::disposeComponent(m_xSet);
+    invalidateRow();
 
     if(isBeforeFirst() || isAfterLast() || !m_xStatement.is())
         return;
@@ -1371,12 +1413,26 @@ void SAL_CALL OKeySet::refreshRow() throw(SQLException, RuntimeException)
         else
             OSL_FAIL("m_rRowCount got out of sync: non-empty m_aKeyMap, but m_rRowCount <= 0");
 
-        if (!isAfterLast())
+        if (m_aKeyIter == m_aKeyMap.end())
         {
-            // it was the last row, but there may be another one to fetch
-            fetchRow();
+            ::comphelper::disposeComponent(m_xSet);
+            if (!isAfterLast())
+            {
+                // it was the last fetched row,
+                // but there may be another one to fetch
+                if (!fetchRow())
+                {
+                    // nope, that really was the last
+                    m_aKeyIter = m_aKeyMap.end();
+                    OSL_ENSURE(isAfterLast(), "fetchRow() failed but not isAfterLast()!");
+                }
+            }
+            // Now, either fetchRow has set m_xRow or isAfterLast()
+        }
+        else
+        {
+            refreshRow();
         }
-        refreshRow();
     }
     else
     {
@@ -1395,22 +1451,38 @@ sal_Bool OKeySet::fetchRow()
     if ( bRet )
     {
         ORowSetRow aKeyRow = new connectivity::ORowVector< ORowSetValue >((*m_pKeyColumnNames).size() + m_pForeignColumnNames->size());
+        ORowSetRow aFullRow = new connectivity::ORowVector< ORowSetValue >(m_pColumnNames->size());
+
+        // Fetch the columns only once and in order, to satisfy restrictive backends such as ODBC
+        const int cc = m_xSetMetaData->getColumnCount();
+        connectivity::ORowVector< ORowSetValue >::Vector::iterator aFRIter = aFullRow->get().begin();
+        // Column 0 is reserved for the bookmark; unused here.
+        ++aFRIter;
+        BOOST_STATIC_ASSERT_MSG(sizeof(int) >= sizeof(sal_Int32), "At least a 32 bit word expecteed");
+        for (int i = 1; i <= cc; ++i, ++aFRIter )
+        {
+            aFRIter->fill(i, m_xSetMetaData->getColumnType(i), m_xDriverRow);
+        }
+
+        ::comphelper::disposeComponent(m_xSet);
+        m_xRow.set(new OPrivateRow(aFullRow->get()));
+
         connectivity::ORowVector< ORowSetValue >::Vector::iterator aIter = aKeyRow->get().begin();
-        // first fetch the values needed for the key columns
+        // copy key columns
         SelectColumnsMetaData::const_iterator aPosIter = (*m_pKeyColumnNames).begin();
         SelectColumnsMetaData::const_iterator aPosEnd = (*m_pKeyColumnNames).end();
         for(;aPosIter != aPosEnd;++aPosIter,++aIter)
         {
             const SelectColumnDescription& rColDesc = aPosIter->second;
-            aIter->fill(rColDesc.nPosition, rColDesc.nType, m_xDriverRow);
+            aIter->fill(rColDesc.nPosition, rColDesc.nType, m_xRow);
         }
-        // now fetch the values from the missing columns from other tables
+        // copy missing columns from other tables
         aPosIter = (*m_pForeignColumnNames).begin();
         aPosEnd  = (*m_pForeignColumnNames).end();
         for(;aPosIter != aPosEnd;++aPosIter,++aIter)
         {
             const SelectColumnDescription& rColDesc = aPosIter->second;
-            aIter->fill(rColDesc.nPosition, rColDesc.nType, m_xDriverRow);
+            aIter->fill(rColDesc.nPosition, rColDesc.nType, m_xRow);
         }
         m_aKeyIter = m_aKeyMap.insert(OKeySetMatrix::value_type(m_aKeyMap.rbegin()->first+1,OKeySetValue(aKeyRow,::std::pair<sal_Int32,Reference<XRow> >(0,NULL)))).first;
     }
@@ -1419,13 +1491,18 @@ sal_Bool OKeySet::fetchRow()
     return bRet;
 }
 
-void OKeySet::fillAllRows()
+bool OKeySet::fillAllRows()
 {
     RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen@sun.com", "OKeySet::fillAllRows" );
-    if(!m_bRowCountFinal)
+    if(m_bRowCountFinal)
+    {
+        return false;
+    }
+    else
     {
         while(fetchRow())
             ;
+        return true;
     }
 }
 // XRow

dbaccess/source/core/api/KeySet.hxx

@@ -131,8 +131,10 @@ namespace dbaccess
         void copyRowValue(const ORowSetRow& _rInsertRow,ORowSetRow& _rKeyRow,sal_Int32 i_nBookmark);
 
         ::com::sun::star::uno::Reference< ::com::sun::star::container::XNameAccess > getKeyColumns() const;
-        void fillAllRows();
+        // returns true if it did any work
+        bool fillAllRows();
         sal_Bool fetchRow();
+        void invalidateRow();
 
         void impl_convertValue_throw(const ORowSetRow& _rInsertRow,const SelectColumnDescription& i_aMetaData);
         void initColumns();