wallet: refactor: dedup sqlite statement preparations/deletions #21540

theStack commented at 1:17 PM on March 28, 2021: member

This refactoring PR deduplicates repeated SQLite statement preparation calls (sqlite3_prepare_v2(...)) / deletions (sqlite3_finalize(...)) and its surrounding logic by putting each prepared statement and its corresponding text representation into a ~std::map~ ~std::array~ std::vector. This should be more readable and less error-prone, e.g. in case an additional statement needs to be added in the future or the error handling has to be adapted.

fanquake added the label Wallet on Mar 28, 2021

fanquake added the label Refactoring on Mar 28, 2021

S3RK commented at 6:36 AM on March 29, 2021: member

Code review ACK 966089a

Empact commented at 5:03 AM on March 30, 2021: member

Concept ACK - the resulting code is significantly more readable IMO and this makes the consistent treatment obvious and enforced.

Logically, the underlying values in statements are not associated as in a map (i.e. the indexing linking the values is not relevant - one is not logically a key and the other a value). A std::array<std::tuple> would seem to be more logically consistent.

theStack force-pushed on Mar 30, 2021

theStack commented at 11:50 AM on March 30, 2021: member

Thanks for reviewing!

Logically, the underlying values in statements are not associated as in a map (i.e. the indexing linking the values is not relevant - one is not logically a key and the other a value). A std::array<std::tuple> would seem to be more logically consistent.

Agreed, using std::map here is like using a sledgehammer to crack a nut 😄 I changed the code to use std::array<std::pair<...,...>, N> now -- fortunately the loop code doesn't need to be changed at all. The only drawback is that we need to specify the size N of the array as second template parameter (std::make_array would help here, but that is not in the standard yet as far as I know), but I think we can live with that.

S3RK commented at 7:04 AM on March 31, 2021: member

reACK 0df8012. The only change is replacing map with array

in src/wallet/sqlite.cpp:80 in 0df8012217 outdated

  99 | +    const std::array<std::pair<sqlite3_stmt**, const char*>, 5> statements {{
 100 | +        { &m_read_stmt,      "SELECT value FROM main WHERE key = ?" },
 101 | +        { &m_insert_stmt,    "INSERT INTO main VALUES(?, ?)" },
 102 | +        { &m_overwrite_stmt, "INSERT or REPLACE into main values(?, ?)" },
 103 | +        { &m_delete_stmt,    "DELETE FROM main WHERE key = ?" },
 104 | +        { &m_cursor_stmt,    "SELECT key, value FROM main" }

MarcoFalke commented at 11:45 AM on April 2, 2021:

        { &m_cursor_stmt,    "SELECT key, value FROM main" },

also could clang-format

theStack commented at 1:58 PM on April 2, 2021:

Thanks, done.

in src/wallet/sqlite.cpp:75 in 0df8012217 outdated

  94 | -        }
  95 | -    }
  96 | -    if (!m_cursor_stmt) {
  97 | -        if ((res = sqlite3_prepare_v2(m_database.m_db, "SELECT key, value FROM main", -1, &m_cursor_stmt, nullptr)) != SQLITE_OK) {
  98 | -            throw std::runtime_error(strprintf("SQLiteDatabase: Failed to setup SQL statements : %s\n", sqlite3_errstr(res)));
  99 | +    const std::array<std::pair<sqlite3_stmt**, const char*>, 5> statements {{

MarcoFalke commented at 11:45 AM on April 2, 2021:

    const std::vector<std::pair<sqlite3_stmt**, const char*>> statements {{

You can use vector to avoid having to type the size

theStack commented at 1:58 PM on April 2, 2021:

Done.

MarcoFalke commented at 11:46 AM on April 2, 2021: member

Any reason to change this method, but not the Close method?

wallet: refactor: dedup sqlite statement preparations 9a3670930e

wallet: refactor: dedup sqlite statement deletions ea19cc844e

theStack force-pushed on Apr 2, 2021

theStack renamed this:
~~wallet: refactor: dedup sqlite statement preparations~~
wallet: refactor: dedup sqlite statement preparations/deletions
on Apr 2, 2021

theStack commented at 1:59 PM on April 2, 2021: member

Took in the suggestions by MarcoFalke (https://github.com/bitcoin/bitcoin/pull/21540#pullrequestreview-626939176): applied clang-format, changed table type from std::array to std::vector to avoid typing the size and add another commit to also deduplicate the deletion of statements (sqlite3_finalize(...)). Thanks for reviewing!

achow101 commented at 3:59 PM on April 2, 2021: member

ACK ea19cc844e780b29825b26aee321204be981a3ae

meshcollider commented at 5:00 AM on April 6, 2021: contributor

utACK ea19cc844e780b29825b26aee321204be981a3ae

fanquake commented at 6:17 AM on April 7, 2021: member

It was while testing this I noticed #21628. However the issue is also present in master.

fanquake merged this on Apr 7, 2021

fanquake closed this on Apr 7, 2021

sidhujag referenced this in commit ef8cd4cc48 on Apr 7, 2021

theStack deleted the branch on Jul 31, 2021

DrahtBot locked this on Aug 18, 2022