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

[Daniel Vrátil]

> On Nov. 18, 2014, 10:29 a.m., Daniel Vrátil wrote:
> > server/src/storage/datastore.h, line 250
> > <https://git.reviewboard.kde.org/r/121120/diff/1/?file=328452#file328452line250>
> >
> >     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?
> 
> Panos Christeas wrote:
>     I think it is needed: query builders use database() to get an instance (in their initializers), which must have an active connection. If this happens too early, they would get a closed QSqlDatabase and bork. I think this is what fixed my stability issues.
> 
> Daniel Vrátil wrote:
>     But QueryBuilders are only used from Handler - at this point the connection is already opened: either because we re-opened the connection in slotNewData(), or because the Handler called DataStore::self() for the very first time, creating and opening the connection anyway.
> 
> Panos Christeas wrote:
>     Well, I guess the problem lies in this reviewboard! (it doesn't allow you to see the full picture, reasoned commits)
>     
>     In commit https://github.com/xrg/akonadi/commit/1463d9eb1984425a7b I have tried removing the initial `open()` , just to emulate early the case of re-using a datastore.
>     
>     Then, as stated above, it turned out that `database()` calls would be made before `slotNewData()` . Somehow, in a stack of initializers. I agree this shall *not* happen. In your development code, you would put an assert, and crash the server until you catch that case. In my case, I just wanted to run akonadi reliably, so converted `database()` to a non-const, opener.

Right, reviewboard only supports reviewing single patches, not patch series. But it's completely OK to upload a single diff between your branch and upstream 1.13 branch (assuming your branch is rebased on latest 1.13). This way I can review all your changes in one go (see the big picture ;-)).


- Daniel


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


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/