I reproduced this behaviour on pc Debian x86-64 on master branch (updated today).
No error/warning messages in console.
HSQL dataengine.

There's something wrong in how the window is populated in function dbaccess::ORowSetCache::fill (file dbaccess/source/core/api/RowSetCache.cxx), but I don't think that the problem is in that function; it is basically a loop of

    for(; _bCheck && _aIter != _aEnd; _aIter++, _nPos++)
    {
        m_pCacheSet->fillValueRow(*_aIter, _nPos);
        _bCheck = m_pCacheSet->next();
    }

Note that the *first* execution of this loop gives correct data; this shows e.g. if you scroll back to the *first* row in the table, that first row is displayed correctly.

Subsequent executions of the loop are fucked up and fill in wrong data (from another row than expected).

I followed the call of fillValueRow, it executes correctly but with wrong data. It seems to me that m_pCacheSet is not positioned correctly, that's it. This would mean the bug is in the call to m_pCacheSet->next(). The latter is basically a delegation to m_xDriverSet->next(), so the problem must be there. 

Ah, this is an OKeySet... Looking at OKeySet::next()... Yup, I think I nailed it.

Testing...

*** Bug 49522 has been marked as a duplicate of this bug. ***

Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "features/base-preview":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e0ef49f034561fcfb024482c1e532c9cae6f5d14&g=features/base-preview

fdo#48345 need to refresh row also when not m_bRowCountFinal

Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=8fca982829c03995e74faf17e5777b3aa61b91dc

fdo#48345 need to refresh row also when not m_bRowCountFinal

Requested review for 3.5 branch... May take a few days.

FWIW the offending lines appear to come from http://cgit.freedesktop.org/libreoffice/core/commit/?id=1ae17f5b03cc14844fb600ca3573a96deb37ab3b so maybe its worth seeing if https://issues.apache.org/ooo/show_bug.cgi?id=102625 is still fixed now

Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "libreoffice-3-5":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=222ba799ad57b0ff93ac103c8af65c706ca4cf37&g=libreoffice-3-5

fdo#48345 need to refresh row also when not m_bRowCountFinal


It will be available in LibreOffice 3.5.5.

(In reply to comment #8)
> Lionel Elie Mamane committed a patch related to this issue.
> It has been pushed to "libreoffice-3-5":
> 
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=222ba799ad57b0ff93ac103c8af65c706ca4cf37&g=libreoffice-3-5
> 
> fdo#48345 need to refresh row also when not m_bRowCountFinal
> 
> 
> It will be available in LibreOffice 3.5.5.


Hi all,

How will this impact db performance, especially with Java connections ? 

Ocke's solution, which caused this display problem in the first place, was only a partial solution to the problem that OOo and LO load the whole dataset into memory even though this is not (on the whole) required. 


Alex

(In reply to comment #7)
> FWIW the offending lines appear to come from
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=1ae17f5b03cc14844fb600ca3573a96deb37ab3b
> so maybe its worth seeing if
> https://issues.apache.org/ooo/show_bug.cgi?id=102625 is still fixed now

My analysis is that #i102625# was *never* really fixed in whole; in the referenced commit, the committer (Ocke) (claims he) fixed a *different* performance problem than what the reporter was complaining about.

(In reply to comment #9)
> (In reply to comment #8)

>> http://cgit.freedesktop.org/libreoffice/core/commit/?id=222ba799ad57b0ff93ac103c8af65c706ca4cf37&g=libreoffice-3-5

>> fdo#48345 need to refresh row also when not m_bRowCountFinal

> How will this impact db performance, especially with Java connections ?

What I'm sure about: my patch does not lead to any requerying if the data is already in LibreOffice memory. Basically, I call refreshRow more often. But this code near the top of refreshRow:

    if ( m_aKeyIter->second.second.second.is() )
    {
        m_xRow = m_aKeyIter->second.second.second;
        return;
    }

says that if the data is already in memory, the m_xRow reference is just changed to point to it. No refetching. So there is no additional database traffic, unless the needed data is not in memory.

> Ocke's solution, which caused this display problem in the first place, was only
> a partial solution to the problem that OOo and LO load the whole dataset into
> memory even though this is not (on the whole) required.

I don't think it changes what OOo/LibO stores in memory; maybe it removes duplicates (things that were stored twice are now stored once? Only in RowSetCache instead of also in KeySet?). It changes the RowSetCache/KeySet interaction so that RowSetCache tells KeySet not to bother to refresh the row (and potentially requery the DB) under some circumstances (when it calls FOO_checked(..., sal_False)). I suppose it is when it knows it will never ask the KeySet for that data anyway, because it has it in cache? I'm rather suspicious of that, basically RowSetCache tells KeySet "If *anybody* asks, give them wrong data; that's OK because I won't ask". Two problems:

 - Will anybody else than the RowSetCache ask? Maybe not, but I'm not sure.

 - That's not robust code. If RowSetCache asks KeySet for that data (subtle bug), we have a "wrong data" bug.

However, I have no smoking gun for my suspicions.

(In reply to comment #10)
> 
> What I'm sure about: my patch does not lead to any requerying if the data is
> already in LibreOffice memory. Basically, I call refreshRow more often. But
> this code near the top of refreshRow:
> 
>     if ( m_aKeyIter->second.second.second.is() )
>     {
>         m_xRow = m_aKeyIter->second.second.second;
>         return;
>     }
> 
> says that if the data is already in memory, the m_xRow reference is just
> changed to point to it. No refetching. So there is no additional database
> traffic, unless the needed data is not in memory.

Bug 47520 reports that the condition in the if-statement can fail with a past-the-end iterator and a SIGABRT.  Perhaps it would be helpful to look at that bug report.

(In reply to comment #11)
> (In reply to comment #10)

>>     if ( m_aKeyIter->second.second.second.is() )

> Bug 47520 reports that the condition in the if-statement can fail with a
> past-the-end iterator and a SIGABRT.  Perhaps it would be helpful to look at
> that bug report.

I see no reference to that if-statement in bug 47520. Maybe you meant another bug?

(In reply to comment #12)
> I see no reference to that if-statement in bug 47520. Maybe you meant another
> bug?

Silly me, I meant bug 50372 "BASE SIGABRT: dereference a past-the-end iterator".  I am sorry for the misdirection.

(In reply to comment #10)
> (In reply to comment #9)
>> How will this impact db performance, especially with Java connections ?

> What I'm sure about: my patch does not lead to any requerying if the data is
> already in LibreOffice memory.

I'm starting to get a glimpse how complex the issue is. Here's my current understanding:

What I said before is technically true, *but* the data USUALLY IS NOT in LibO memory. It is only we inserted / updated / ... that data OURSELVES. So *yes*, this change leads to more requerying again and essentially reverts the optimisation seen in the buglog of #i102625#. This optimisation was: if we just fetched a row in our "SELECT * FROM ...", then use the driver row directly to serve subsequent column requests. That optimisation was buggily implemented and pointed to the driver row even when one did *not* just fetch a row, thereby using another row.

OTOH, this optimisation leads to problems with ODBC drivers (see e.g. bug 47520), because one is not allowed to fetch the same data twice: primary key columns are fetched once by the KeySet for its internal use, and then once by the wrapping windowed cache (ORowSetCache).

So basically, in KeySet we have to read the *whole* row *once*, cache the result, and then use that to fill the internals of KeySet (the primary key columns) and to answer any getShort/getLong/getString/... query.

We *could* make the cache permanent (store it in m_aKeyIter->second.second.second), but that would take undue amounts of memory for big tables, and it feels particularly stupid that we have an intelligent "windowed" cache on top of that, as we would cache the whole table anyway... Why have another cache on top of that? So I'll make that cache invalid (memory freed) as soon as one moves in the KeySet (with absolute(i), next(), previous(), first(), last(), etc).

This will get us back the optimisation that we don't stupidly read just the primary key a row in "SELECT * FROM table" and then immediatly do a new query "SELECT * FROM table WHERE primary_key=current_row" to fetch the rest of the data in that row, which is what was done in #i102625# was ultimately about. OTOH, if we have moved away from the row, we *will* refetch it.

This looks to me like the right balance between memory consumption and network (LibO<->DataBase) traffic.

That "optimisation done right" will IMO *not* go into 3.5, though. Only 3.6.

(In reply to comment #14)
> (In reply to comment #10)
>> (In reply to comment #9)

>>> How will this impact db performance, especially with Java connections ?

> *yes*, this change leads to more requerying again and essentially reverts the
> optimisation seen in the buglog of #i102625#.

> So basically, in KeySet we have to read the *whole* row *once*, cache the
> result, and then use that to fill the internals of KeySet (the primary key
> columns) and to answer any getShort/getLong/getString/... query.

I just pushed a reimplementation of that optimisation to master (LibreOffice 3.6 beta). Testing appreciated.

*** Bug 50721 has been marked as a duplicate of this bug. ***