Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve error checking in SQLite database connection (again) #10553

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Improve error checking in SQLite database connection (again) #10553

merged 4 commits into from
Jun 17, 2024

Conversation

simularis
Copy link
Contributor

Pull request overview

  • Follow up to Improve error checking in SQLite database connection #10483
  • Proactive effort to avoid failure modes for opening SQLite connection:
    • Refactor SQLiteProcedures class member variables to store only one pointer to the same sqlite3 connection (better encapsulation)
    • Avoid calling sqlite3_errmsg after sqlite3_close. Such a call sequence always yields the unhelpful error message "bad parameter or other API misuse".
    • Avoid creating a sqlite3 journal file while testing for access to create/write/lock file.

Suggested tag: Defect

Discussion

The SQliteProcedures constructor tests for access to create/write/lock a sqlite3 database file. Unless the calling code sets journal_mode = OFF, the first call to sqlite3_exec opens a database file (dbName) and a journal file at dbName + "-journal". However, testing access to the journal file is not necessary since the inheriting class SQLite sets journal_mode = OFF and never uses it. In some cases, failing to turn off the journal file could lead to an error. Namely, on Windows, file paths may be limited to 260 characters, depending on registry settings. While the database file may be within the path length limit, the journal file may exceed it. Then, EnergyPlus would return an error message blaming sqlite.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

simularis added 3 commits June 8, 2024 19:36
@simularis
Encapsulate raw pointer sqlite3 m_connection
af8aef8
@simularis
Avoid accessing sqlite3 connection after sqlite3_close()
bedc515 
After calling sqlite3_close(), the call to sqlite3_errmsg()
is guaranteed to yield useless message "bad parameter or other API misuse"
@simularis
Set journal_mode OFF to avoid creating sqlite3 journal file
8fae2de 
Creating a journal file could decrease the character limit for
folder paths on Windows. Currently, the only inheriting class
uses journal_mode = OFF, so it is not necessary to test for
access to create and lock a journal file. Confer SQLite::SQLite().
simularis
simularis commented Jun 9, 2024
View reviewed changes
src/EnergyPlus/SQLiteProcedures.hh
Comment on lines -107 to 108
sqlite3 *m_connection;
std::shared_ptr<sqlite3> m_db;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that m_db is a shared pointer to m_connection, created in the SQLiteProcedures constructor. There is no need to store m_connection for direct access.

src/EnergyPlus/SQLiteProcedures.cc
@@ -2803,7 +2808,7 @@ int SQLiteProcedures::sqliteResetCommand(sqlite3_stmt *stmt)

bool SQLiteProcedures::sqliteWithinTransaction()
{
return (sqlite3_get_autocommit(m_connection) == 0);
return (sqlite3_get_autocommit(m_db.get()) == 0);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use the shared_ptr instead of the pointer it owns.

src/EnergyPlus/SQLiteProcedures.cc
Comment on lines 2599 to +2610
SQLiteProcedures::SQLiteProcedures(std::shared_ptr<std::ostream> const &errorStream, std::shared_ptr<sqlite3> const &db)
: m_writeOutputToSQLite(true), m_errorStream(errorStream), m_connection(nullptr), m_db(db)
: m_writeOutputToSQLite(true), m_errorStream(errorStream), m_db(db)
{
}

SQLiteProcedures::SQLiteProcedures(std::shared_ptr<std::ostream> const &errorStream,
bool writeOutputToSQLite,
fs::path const &dbName,
fs::path const &errorFilePath)
: m_writeOutputToSQLite(writeOutputToSQLite), m_errorStream(errorStream), m_connection(nullptr)
: m_writeOutputToSQLite(writeOutputToSQLite), m_errorStream(errorStream)
{
sqlite3 *m_connection = nullptr;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of initializing member variable, initialize m_connection as a local variable.

src/EnergyPlus/SQLiteProcedures.cc
sqlite3_close(m_connection);
if (rc) {
*m_errorStream << "SQLite3 message, can't get exclusive lock to edit database: " << sqlite3_errmsg(m_connection) << std::endl;
*m_errorStream << "SQLite3 message, can't get exclusive lock to edit database: " << zErrMsg << std::endl;
ok = false;
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After sqlite3_close(), we should not call sqlite3_errmsg(). Instead, the error message was already returned by a previous function call.

src/EnergyPlus/SQLiteProcedures.cc
rc = sqlite3_exec(m_connection, "PRAGMA journal_mode = OFF;", nullptr, 0, &zErrMsg);
if (!rc) {
rc = sqlite3_exec(m_connection, "CREATE TABLE Test(x INTEGER PRIMARY KEY)", nullptr, 0, &zErrMsg);
}
sqlite3_close(m_connection);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we turn off the journal mode then execute a query.

@dareumnam
Copy link
Collaborator

dareumnam commented Jun 13, 2024 •
edited
Loading

Thanks for the PR. I looked at some of the CI testing diffs, and it seems like this branch's fixes are:

  • Omitting some output variables like:
    • Zone Wetbulb Globe Temperature (according to RDD diffs)
    • Solar Transmittance at Normal Incidence, Visible Transmittance at Normal Incidence In WindowConstruction (according to EIO diffs)
    • Minimum/maximum Number of People for All Day Types, Minimum/Maximum Number of People for Weekdays, Weekends /Holidays, or Winter/Summer Design Days, etc In Internal Gains Nominal (according to EIO diffs)
  • Changing some node number indices (according to BND diffs)
    I think we should ensure there are no output changes after these fixes.

@dareumnam dareumnam added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Jun 13, 2024
@simularis
Copy link
Contributor Author

I guess the CI tests do not merge the develop branch? My local copy of the develop branch is out of date, so I'll merge in the latest develop branch. Otherwise, the commits are only related to SQLiteProcedures class and do not impact any other features.

@simularis
Merge pull request #1 from NREL/develop
1d6293b 
Merge NREL/EnergyPlus:develop
@dareumnam
Copy link
Collaborator

CI does not automatically merge the latest develop branch. We've had several PRs merged in recent days, which probably caused test diffs. Now this branch is up to date, and all CI testing looks good. This seems like a good addition to enhance SQLite database connection error checking.

@dareumnam
Copy link
Collaborator

This PR looks fully clean and ready to go. @Myoldmopar

@Myoldmopar Myoldmopar added the Defect Includes code to repair a defect in EnergyPlus label Jun 17, 2024
@Myoldmopar Myoldmopar self-assigned this Jun 17, 2024
@Myoldmopar Myoldmopar merged commit 3c8844f into NREL:develop Jun 17, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Myoldmopar Myoldmopar

Labels
Defect Includes code to repair a defect in EnergyPlus Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@simularis @dareumnam @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c @Myoldmopar