API change to 3.0.91: improve memory management of raw SQL execution and preparing
ClosedPublic

Authored by staniek on May 8 2017, 3:27 PM.

Details

Summary
  • Change KDbSqlResult* KDbConnection::executeSQL to QSharedPointer<KDbSqlResult> KDbConnection::prepareSql (fixes risks of memory leaks and unfinished execution when resulting object is ignored)
  • Change bool KDbConnection::executeVoidSQL to bool KDbConnection::executeSql (also in Kexi)
  • Change KDbConnection::insertRecord to return QSharedPointer<KDbSqlResult> (fixes risks of memory leaks and unfinished execution when resulting object is ignored)
  • Change KDbConnection::recentSQLString() to recentSqlString()
  • Change KDbSqlRecord* KDbSqlResult::fetchRecord() to QSharedPointer<KDbSqlRecord> fetchRecord()
  • Change KDbSqlResult* KDbPreparedStatementInterface::execute() to QSharedPointer<KDbSqlResult> KDbPreparedStatementInterface::execute()
  • Change KDb::lastInsertedAutoIncValue(KDbSqlResult *result, ...) to KDb::lastInsertedAutoIncValue(QSharedPointer<KDbSqlResult>, ...)
  • Update drivers accordingly
  • KDb & Kexi: Replace KDbConnection::executeVoidSQL uses with KDbConnection::executeSql and Replace KDbConnection::executeSQL uses with KDbConnection::prepareSql where needed (fix memory leak and unfinished execution)
Test Plan

Run all autotests of KDb, build and try Kexi with sqlite/pgsql/mysql projects.

Diff Detail

Repository
R15 KDb
Branch
arcpatch-D5771
Lint
No Linters Available
Unit
No Unit Test Coverage
staniek created this revision.May 8 2017, 3:27 PM
Restricted Application added a project: KDb. · View Herald TranscriptMay 8 2017, 3:27 PM
staniek updated this revision to Diff 14293.May 8 2017, 3:29 PM

Add version info

staniek edited the summary of this revision. (Show Details)
staniek edited the test plan for this revision. (Show Details)

This is not a review, it's a few impressions :-)

src/KDb.h
115–130

Shared pointer make sense when it's passed by value to increase ref count.

src/KDbConnection.cpp
1074–1075

Pointer is passed when argument can be optional i.e. nullptr, by reference when it can't be optional. Respecting this rule code base became cleaner.

src/drivers/mysql/MysqlPreparedStatement.cpp
140

sqlite bind in mysql driver - it looks quite strange :) It's not only here.

staniek marked an inline comment as done.May 8 2017, 6:00 PM
staniek added inline comments.
src/KDb.h
115–130

Thanks Anthony, definitely that was too mechanical change.

src/KDbConnection.cpp
1074–1075

What you say works but we're using the Qt API design guidelines. Like Q*Event* is used everywhere in QWidget::*Event(QEvent *) even while it's required.

In other words, do you know a place in, say, QtSql and further in Qt or even KF5 that uses nonconst-references for the mentioned reason?

Another topic is missing assertions or checks for such code.

src/drivers/mysql/MysqlPreparedStatement.cpp
140

Thanks, currently it's ifdefed out, KDB_USE_MYSQL_STMT is off. Code kept for reference here and changes made mechanically.

staniek updated this revision to Diff 14303.May 8 2017, 6:06 PM
staniek marked an inline comment as done.
staniek edited the summary of this revision. (Show Details)
  • Pass the KDbSqlResult by value to KDb::lastInsertedAutoIncValue
staniek added inline comments.May 8 2017, 6:07 PM
src/KDb.h
115–130

Done, the param is now optional. Feel free to review the other code :)

staniek updated this revision to Diff 14304.May 8 2017, 6:10 PM

Update the commit message

staniek edited the summary of this revision. (Show Details)May 8 2017, 6:10 PM
piggz added inline comments.May 11 2017, 5:33 PM
src/KDbConnection.h
953

why not returning a qsharedpointer here?

staniek added inline comments.May 12 2017, 11:04 AM
src/KDbConnection.h
953

I used to do that initially but since this is not a user API, for performance I decided to return raw pointer here. Ownership is returned here to the caller so it's a clear situation.

piggz accepted this revision.May 12 2017, 5:34 PM
This revision is now accepted and ready to land.May 12 2017, 5:34 PM
This revision was automatically updated to reflect the committed changes.