[Libreoffice-commits] .: Branch 'features/base-preview' - 5 commits - dbaccess/source
Lionel Elie Mamane
lmamane at kemper.freedesktop.org
Mon Jun 4 09:52:12 PDT 2012
dbaccess/source/core/api/KeySet.cxx | 170 +++++++++++++++++++++++--------
dbaccess/source/core/api/KeySet.hxx | 4
dbaccess/source/core/api/RowSetCache.cxx | 10 +
3 files changed, 140 insertions(+), 44 deletions(-)
New commits:
commit c2c329fbd01093cd26a71a91de2d3803a535d76f
Author: Lionel Elie Mamane <lionel at mamane.lu>
Date: Mon Jun 4 17:54:30 2012 +0200
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 1ae17f5b03cc14844fb600ca3573a96deb37ab3b 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 1ae17f5b03cc14844fb600ca3573a96deb37ab3b.
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
diff --git a/dbaccess/source/core/api/KeySet.cxx b/dbaccess/source/core/api/KeySet.cxx
index b943d10..2709b2e 100644
--- a/dbaccess/source/core/api/KeySet.cxx
+++ b/dbaccess/source/core/api/KeySet.cxx
@@ -379,6 +379,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 at sun.com", "OKeySet::getBookmark" );
@@ -404,10 +410,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)
@@ -1120,7 +1127,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();
@@ -1168,8 +1180,7 @@ void SAL_CALL OKeySet::beforeFirst( ) throw(SQLException, RuntimeException)
RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen at 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)
@@ -1178,8 +1189,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)
@@ -1188,8 +1198,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();
@@ -1204,12 +1220,17 @@ sal_Bool OKeySet::last_checked( sal_Bool i_bFetchRow)
{
RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen at 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();
}
@@ -1231,10 +1252,11 @@ sal_Bool OKeySet::absolute_checked( sal_Int32 row,sal_Bool i_bFetchRow )
RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen at 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--;
@@ -1243,18 +1265,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
{
@@ -1263,8 +1299,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();
}
@@ -1289,6 +1330,8 @@ sal_Bool OKeySet::previous_checked( sal_Bool i_bFetchRow )
--m_aKeyIter;
if ( i_bFetchRow )
refreshRow();
+ else
+ invalidateRow();
}
return m_aKeyIter != m_aKeyMap.begin();
}
@@ -1345,8 +1388,7 @@ void SAL_CALL OKeySet::refreshRow() throw(SQLException, RuntimeException)
{
RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen at sun.com", "OKeySet::refreshRow" );
- m_xRow = NULL;
- ::comphelper::disposeComponent(m_xSet);
+ invalidateRow();
if(isBeforeFirst() || isAfterLast() || !m_xStatement.is())
return;
@@ -1372,12 +1414,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())
+ {
+ ::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
{
- // it was the last row, but there may be another one to fetch
- fetchRow();
+ refreshRow();
}
- refreshRow();
}
else
{
@@ -1396,22 +1452,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;
}
@@ -1420,13 +1492,18 @@ sal_Bool OKeySet::fetchRow()
return bRet;
}
-void OKeySet::fillAllRows()
+bool OKeySet::fillAllRows()
{
RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen at sun.com", "OKeySet::fillAllRows" );
- if(!m_bRowCountFinal)
+ if(m_bRowCountFinal)
+ {
+ return false;
+ }
+ else
{
while(fetchRow())
;
+ return true;
}
}
// XRow
diff --git a/dbaccess/source/core/api/KeySet.hxx b/dbaccess/source/core/api/KeySet.hxx
index 90ba033..b98086d 100644
--- a/dbaccess/source/core/api/KeySet.hxx
+++ b/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();
commit f46b2cc39a520d00b6bda1cc0071000d179391c4
Author: Lionel Elie Mamane <lionel at mamane.lu>
Date: Mon Jun 4 17:41:33 2012 +0200
Need to refresh row after moving to bookmark!
Change-Id: Ia8d12d02829087309e248506a7d3b0f94b5a425e
diff --git a/dbaccess/source/core/api/KeySet.cxx b/dbaccess/source/core/api/KeySet.cxx
index e89793a..b943d10 100644
--- a/dbaccess/source/core/api/KeySet.cxx
+++ b/dbaccess/source/core/api/KeySet.cxx
@@ -392,6 +392,8 @@ sal_Bool SAL_CALL OKeySet::moveToBookmark( const Any& bookmark ) throw(SQLExcept
RTL_LOGFILE_CONTEXT_AUTHOR( aLogger, "dbaccess", "Ocke.Janssen at sun.com", "OKeySet::moveToBookmark" );
m_bInserted = m_bUpdated = m_bDeleted = sal_False;
m_aKeyIter = m_aKeyMap.find(::comphelper::getINT32(bookmark));
+ if (m_aKeyIter != m_aKeyMap.end())
+ refreshRow();
return m_aKeyIter != m_aKeyMap.end();
}
commit 49701fdc5f9533b8e146dfc1d9a4c7cbfd4c1fc8
Author: Lionel Elie Mamane <lionel at mamane.lu>
Date: Mon Jun 4 17:40:30 2012 +0200
Cleanup m_xSet in destructor
Change-Id: I3d7023fcb1857da1ef107a8af0d373b9ca464f03
diff --git a/dbaccess/source/core/api/KeySet.cxx b/dbaccess/source/core/api/KeySet.cxx
index dbb9086..e89793a 100644
--- a/dbaccess/source/core/api/KeySet.cxx
+++ b/dbaccess/source/core/api/KeySet.cxx
@@ -95,6 +95,22 @@ namespace
}
}
}
+
+ template < typename T > inline void tryDispose( Reference<T> &r )
+ {
+ try
+ {
+ ::comphelper::disposeComponent(r);
+ }
+ catch(const Exception&)
+ {
+ r = NULL;
+ }
+ catch(...)
+ {
+ OSL_FAIL("Unknown Exception occurred");
+ }
+ }
}
DBG_NAME(OKeySet)
@@ -125,18 +141,9 @@ OKeySet::OKeySet(const connectivity::OSQLTable& _xTable,
OKeySet::~OKeySet()
{
- try
- {
- ::comphelper::disposeComponent(m_xStatement);
- }
- catch(const Exception&)
- {
- m_xStatement = NULL;
- }
- catch(...)
- {
- OSL_FAIL("Unknown Exception occurred");
- }
+ tryDispose(m_xStatement);
+ tryDispose(m_xSet);
+
m_xComposer = NULL;
DBG_DTOR(OKeySet,NULL);
commit fd356350b8caf8718991b09bbca5a640c7226bd7
Author: Lionel Elie Mamane <lionel at mamane.lu>
Date: Mon Jun 4 17:35:52 2012 +0200
typos in comments
Change-Id: I1dbb1990033602d7909ecdee72b8b699cce44cab
diff --git a/dbaccess/source/core/api/KeySet.cxx b/dbaccess/source/core/api/KeySet.cxx
index 715ca6b..dbb9086 100644
--- a/dbaccess/source/core/api/KeySet.cxx
+++ b/dbaccess/source/core/api/KeySet.cxx
@@ -293,7 +293,7 @@ void OKeySet::construct(const Reference< XResultSet>& _xDriverSet,const ::rtl::O
findTableColumnsMatching_throw(makeAny(m_xTable),m_sUpdateTableName,xMeta,xQueryColumns,m_pKeyColumnNames);
// the first row is empty because it's now easier for us to distinguish when we are beforefirst or first
- // without extra varaible to be set
+ // without extra variable to be set
m_aKeyMap.insert(OKeySetMatrix::value_type(0,OKeySetValue(NULL,::std::pair<sal_Int32,Reference<XRow> >(0,NULL))));
m_aKeyIter = m_aKeyMap.begin();
@@ -434,7 +434,7 @@ Sequence< sal_Int32 > SAL_CALL OKeySet::deleteRows( const Sequence< Any >& rows
aSql.append(m_aComposedTableName);
aSql.append(::rtl::OUString(RTL_CONSTASCII_USTRINGPARAM(" WHERE ")));
- // list all cloumns that should be set
+ // list all columns that should be set
const ::rtl::OUString aQuote = getIdentifierQuoteString();
static ::rtl::OUString aAnd(RTL_CONSTASCII_USTRINGPARAM(" AND "));
static ::rtl::OUString aOr(RTL_CONSTASCII_USTRINGPARAM(" OR "));
commit b88047da258529863317bf7793db6075ab72eee0
Author: Lionel Elie Mamane <lionel at mamane.lu>
Date: Mon Jun 4 17:31:25 2012 +0200
Remove wrong optimisation
fixup of d4ae29a37873843c20fe7d5f5f071f8fb201fed9
after the call to m_pCacheSet->absolute_checked, the data *is* used,
so we cannot anymore exempt m_pCacheSet from giving correct data.
Change-Id: I7d3644ca08ce43cb030a80984605a1f8a8a64211
diff --git a/dbaccess/source/core/api/RowSetCache.cxx b/dbaccess/source/core/api/RowSetCache.cxx
index 83978cd..f8e54e1 100644
--- a/dbaccess/source/core/api/RowSetCache.cxx
+++ b/dbaccess/source/core/api/RowSetCache.cxx
@@ -79,6 +79,14 @@ using namespace ::osl;
#define CHECK_MATRIX_POS(M) OSL_ENSURE(((M) >= static_cast<ORowSetMatrix::difference_type>(0)) && ((M) < static_cast<sal_Int32>(m_pMatrix->size())),"Position is invalid!")
+// This class calls m_pCacheSet->FOO_checked(..., sal_False)
+// (where FOO is absolute, last, previous)
+// when it does not immediately care about the values in the row's columns.
+// As a corollary, m_pCacheSet may be left in an inconsistent state,
+// and all ->fillFOO calls (and ->getFOO) may fail or give wrong results,
+// until m_pCacheSet is moved (or refreshed) again.
+// So always make sure m_pCacheSet is moved or refreshed before accessing column values.
+
DBG_NAME(ORowSetCache)
ORowSetCache::ORowSetCache(const Reference< XResultSet >& _xRs,
@@ -1076,7 +1084,7 @@ sal_Bool ORowSetCache::moveWindow()
aIter = m_pMatrix->begin();
nPos = m_nStartPos + 1;
- bCheck = m_pCacheSet->absolute_checked(nPos, sal_False);
+ bCheck = m_pCacheSet->absolute_checked(nPos, sal_True);
for(; !aIter->is() && bCheck;++aIter, ++nPos)
{
OSL_ENSURE(aIter != m_pMatrix->end(),"Invalid iterator");
More information about the Libreoffice-commits
mailing list