[Kde-pim] Review Request 121120: try to release idle connections to DB, relieve database

Tue Nov 18 09:29:35 GMT 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121120/#review70574
-----------------------------------------------------------

Looks pretty good! Please fix the few issues I raised below (you can ignore the "Remove akDebug()" comments for now), and I'll put it into testing locally.

server/src/connection.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49386>

    Add `Q_ASSERT(m_currentHandler == 0)`. This is a condition that must be true at all times, otherwise it indicates a programming/logical error.

server/src/connection.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49390>

    Remove, please (don't have to do that now, but before we ship this). This is not super important and would cause Akonadi to be overlay chatty :)

server/src/connection.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49380>

    We definitely don't want to close the connection while we are in a transaction - I think that would rollback the transaction.

    On the other hand, nothing should be keeping transaction opened for more than 10 minutes. There is a way for Akonadi clients to start a persistent database transaction (that can span over multiliple commands), but that is internal to the library, so completely under our control. If something is keeping the transaction opened this long between two commands, I would call it a bug :)

    So I would print a big fat warning here if we are in a transaction at this point, but I'd just reset the timer to another 10 minutes (or half the time) and continue /without/ closing the connection.

server/src/connection.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49392>

    I don't think this is necessary.

server/src/connection.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49391>

    Remove, please

server/src/connection.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49387>

    Let's use the storageBackend() call here - otherwise m_backend might not get initialized (not all handlers call it), and so the condition in slotConnectionIdle() would never be evaluated to true.

server/src/connection.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49389>

    Remove, please.

server/src/connection.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49388>

    This will force-open a DB connection even on connections that don't need it, let's try to avoid that.

    Either we could check, whether the DataStore actually exists in this thread (you want a static method in DataStore that returns DataStore::sInstances.hasLocalData()), or have a bool wasConnected variable here in Connection that is set to true in slotConnectionIdle(), so that we know we should reconnect in slotNewData().

    The first approach sounds better to me.

server/src/storage/datastore.h
<https://git.reviewboard.kde.org/r/121120/#comment49385>

    Is this actually needed? Since we will always open the connection at slotNewData() and we make sure we don't close it while in handler, it should never happen that something calls database() is a closed DataStore, right?

server/src/storage/datastore.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49381>

    Add { } please

server/src/storage/datastore.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49382>

    Add {} please

server/src/storage/datastore.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49384>

    This is taken care of in rollbackTransaction() above.

server/src/storage/dbconfigpostgresql.cpp
<https://git.reviewboard.kde.org/r/121120/#comment49383>

    Remove

- Daniel Vrátil

On Nov. 16, 2014, 8:42 a.m., Panos Christeas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121120/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2014, 8:42 a.m.)
> 
> 
> Review request for Akonadi and Daniel Vrátil.
> 
> 
> Repository: akonadi
> 
> 
> Description
> -------
> 
> For Postgres, each DB client connection corresponds to a process
> being spawn and kept alive. May even mean some SHM allocated for
> it, on pre-9.3 versions of postgres.
> On a system (desktop) with many akonadi resources, we may end up
> occupying most/all available Postgres backends just keeping idle
> agents.
> This patch tries to address that, by releasing the DB connection
> for those idle agents. This comes to the expense of some wake-up
> latency (connect, setup db and prepare queries again). It is not
> limited to QPSQL DBs yet, as to explore its effects on the other
> storage engines.
> 
> Note that I had to change the `DataStore::database()` API from a
> const to a regular method, because it would wake-up a connection.
> 
> 
> Diffs
> -----
> 
>   server/src/connection.h 3f1141eec1561196e2a12b03de6c13f8051349db 
>   server/src/connection.cpp 32f10d5d2d76baa737f494fe5c283ea047344983 
>   server/src/storage/datastore.h 395b227c5c085566d755f3636ac4ac20bde97723 
>   server/src/storage/datastore.cpp ae78baba3e3db4937061e52d91bf401393d4c189 
>   server/src/storage/dbconfigpostgresql.cpp ebad22cd7dca935b318241b7428fb0b7ccdee881 
>   server/src/storage/querybuilder.cpp c07905906bdc712d41007ada753c2d821afd0a2d 
> 
> Diff: https://git.reviewboard.kde.org/r/121120/diff/
> 
> 
> Testing
> -------
> 
> Ran for 2 days, several resources.
> 
> 
> Thanks,
> 
> Panos Christeas
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/