First steps towards PostgreSQL integration - #651
gstavrinos wants to merge 17 commits into
Conversation
Tests are yet to be implemented, so the code remains in an early stage, completely untested even for basic functionality
There was a problem hiding this comment.
🟡 Changes recommended
There are multiple confirmed correctness/build issues (schema DDL syntax error, incorrect SELECT result handling via affected_rows(), missing test source file in CMake, and credential logging risk) that must be fixed before it can be safely validated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces an initial PostgreSQL-backed implementation of the FaultStorage backend for ros2_medkit_fault_manager, wiring it into FaultManagerNode via a new storage_type=postgres option and a database_url parameter.
Changes:
- Add
PgFaultStorage(libpqxx-based) with schema initialization and implementations for fault/events, snapshots, near-misses, and rosbag retention APIs. - Extend
FaultManagerNodeto select PostgreSQL storage via parameters. - Update build/package dependencies to include PostgreSQL/libpqxx and add a placeholder GTest target for PostgreSQL storage.
File summaries
| File | Description |
|---|---|
| src/ros2_medkit_fault_manager/src/postgres_fault_storage.cpp | New PostgreSQL FaultStorage implementation and schema creation logic. |
| src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/postgres_fault_storage.hpp | Public header for PgFaultStorage implementing the FaultStorage interface. |
| src/ros2_medkit_fault_manager/src/fault_manager_node.cpp | Adds database_url param and selects PostgreSQL storage when storage_type=postgres. |
| src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp | Stores new database_url_ member. |
| src/ros2_medkit_fault_manager/CMakeLists.txt | Adds PostgreSQL dependency linkage and a PostgreSQL test target (currently missing source). |
| src/ros2_medkit_fault_manager/package.xml | Declares libpqxx dependency. |
Review details
Suppressed comments (4)
src/ros2_medkit_fault_manager/src/postgres_fault_storage.cpp:547
get_fault()usesaffected_rows()on a SELECT result; this can incorrectly return nullopt even when a fault exists. Useres.empty()to test whether the query returned rows.
auto res = tx.exec_params(
"SELECT fault_code, severity, description, first_occurred_ns, last_occurred_ns, occurrence_count, status, "
"reporting_sources, last_passed_ns FROM faults WHERE fault_code = $1",
fault_code);
if (res.affected_rows() == 0) {
src/ros2_medkit_fault_manager/src/postgres_fault_storage.cpp:653
contains()usesaffected_rows()on a SELECT result; that can incorrectly reportfalseeven when the row exists. For SELECT queries, checkres.empty()instead.
auto res = tx.exec_params("SELECT 1 FROM faults WHERE fault_code = $1 LIMIT 1", fault_code);
tx.commit();
return res.affected_rows() > 0;
src/ros2_medkit_fault_manager/src/postgres_fault_storage.cpp:748
- The newest-capture query aliases the column as
max_capture_idbut then readsmax_capture, and also usesaffected_rows()on a SELECT. This can throw at runtime and/or disable trimming. Prefer COALESCE +res.empty()and read the correct alias.
auto res =
tx.exec_params("SELECT MAX(capture_id) AS max_capture_id FROM snapshots WHERE fault_code = $1", fault_code);
int64_t newest_capture = 0;
if (res.affected_rows() > 0) {
newest_capture = res[0]["max_capture"].as<int64_t>();
src/ros2_medkit_fault_manager/src/postgres_fault_storage.cpp:753
- The snapshot-trimming loop uses
count_res.affected_rows() == 0on aSELECT COUNT(*)query; for SELECTs this is not a valid emptiness check and can short-circuit trimming unexpectedly. Usecount_res.empty()(or just read the first row) instead.
auto count_res = tx.exec_params("SELECT COUNT(*) AS sz FROM snapshots WHERE fault_code = $1", fault_code);
if (count_res.affected_rows() == 0 || count_res[0]["sz"].as<size_t>() <= max_snapshots_per_fault_) {
break;
- Files reviewed: 6/6 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # PostgreSQL storage tests | ||
| medkit_add_gtest(test_postgres_storage test/test_postgres_storage.cpp) | ||
| target_link_libraries(test_postgres_storage fault_manager_lib) | ||
| medkit_target_dependencies(test_postgres_storage rclcpp ros2_medkit_msgs) | ||
|
|
| } | ||
|
|
||
| if (storage_type_ == "postgres") { | ||
| RCLCPP_INFO(get_logger(), "Using PostgreSQL fault storage: %s", database_url_.c_str()); |
| CREATE INDEX IF NOT EXISTS idx_snapshots_fault_code ON snapshots(fault_code); | ||
| CREATE INDEX IF NOT EXISTS idx_snapshots_fault_topic ON snapshots(fault_code, topic))"); |
| "first_occurred_ns FROM faults WHERE fault_code = $1", | ||
| fault_code); | ||
|
|
||
| if (res.affected_rows() > 0) { |
| auto res = tx.exec_params("SELECT COUNT(*) FROM rosbag_files WHERE file_path = $1", file_path); | ||
| tx.commit(); | ||
| return res.affected_rows(); |
| for (const auto & r : res) { | ||
| paths.insert(r["file_path"].as<std::string>()); | ||
| } | ||
| removed = res.affected_rows() > 0; |
|
Oh, wow, Copilot came in aggressively! I will consider the LLM's remarks as I am implementing the tests. |
No worries, he is always like that. Use your own judgement, some of his comments are not worth fixing. Happy to take a look. |
|
Hello, I am currently finishing up the unit tests. I plan to update this PR with the new changes tomorrow. Apart from the integration and testing code, I also want to update the documentation. Can you please point me to the required documentation file(s) that need to be updated? Thanks! |
Hi @gstavrinos, two files to update:
That should be it, but if I find anything else, I'll let you know. |
Improved SELECT result handling based on best practices, fixed typos, fixed potential password leaks from printing the database url and other minor issues
|
I have updated the source code, added the unit tests, a docker compose file and relevant documentation points. Some remarks:
I will be waiting for your feedback on the provided material and the next steps towards merging. Thanks again! |
bburda
left a comment
bburda
left a comment
There was a problem hiding this comment.
Thanks for this contribution :)
There are also two bigger things: the PostgreSQL tests do not run in CI, and the fault manager stops on the next fault report after the database restarts. We can open separate issues for them, or you can fix them in this PR. Please also make sure that CI passes.
| find_package(rclcpp REQUIRED) | ||
| find_package(ros2_medkit_msgs REQUIRED) | ||
| find_package(ros2_medkit_serialization REQUIRED) | ||
| find_package(PostgreSQL REQUIRED) |
There was a problem hiding this comment.
Could you put the PostgreSQL backend behind a CMake option that is OFF by default? Right now libpqxx is required for every build, and that breaks builds that never use PostgreSQL:
- Humble has libpqxx 6.4. The new file does not compile there:
connection::close()is protected, astd::vector<std::string>cannot be a query parameter (list_faults()), and<filesystem>is not included. - On Lyrical, 22 of 24 fault_manager tests fail. For example,
test_capture_thread_poolpasses all 12 tests and then aborts at exit withfree(): double free detected in tcache 2. The cause is the libpqxx 7.10.0 package in Ubuntu 26.04. A program with only#include <pqxx/pqxx>andint main() { return 0; }aborts in the same way. If I link libpqxx and leave out the header, it exits normally. - The runtime stage of the Dockerfile in this repository does not install libpqxx.
fault_manager_nodenow needslibpqxx-7.8.so, so the Docker image cannot start the fault manager.
With the option OFF, please build nothing PostgreSQL-related: no find_package, no postgres_fault_storage.cpp, no pqxx link and no test target. In such a build, create_storage() should stop with a clear error when storage_type is postgres. The #include of postgres_fault_storage.hpp in fault_manager_node.cpp needs the same guard. Please also remove <depend>libpqxx-dev</depend> from package.xml, and write the option name and the libpqxx-dev package in the README, because rosdep does not install it without the <depend>.
| // The SQLite backend has to opt into a transaction for this (BEGIN IMMEDIATE) | ||
| // In the PostgreSQL implementation the lock used is a transaction lock, so no extra handling is required apart from | ||
| // updating the tables properly | ||
| pqxx::work tx(*db_conn_); |
There was a problem hiding this comment.
I built this branch and ran the fault manager against PostgreSQL 16. I reported one fault, restarted the database container and reported a second fault. The node stopped on the second report:
terminate called after throwing an instance of 'pqxx::broken_connection'
what(): Lost connection to the database server.
The object opens one pqxx::connection and never reconnects, and the service handlers do not catch storage exceptions. So every database restart or network drop stops the fault manager. If you want to fix this in the PR, two changes are needed:
- In
PgFaultStorage: when a transaction cannot start because the connection is broken, open a new connection and try the call once more. Do not retry onpqxx::in_doubt_error, because the commit may already be applied. - In the node: catch exceptions from
storage_in the service and timer callbacks and return an error response, so the process keeps running while the database is down.
| for (const auto & path : unique_paths) { | ||
| if (path_referenced(path)) { | ||
| continue; | ||
| } | ||
| std::error_code ec; | ||
| std::filesystem::remove_all(path, ec); | ||
| } |
There was a problem hiding this comment.
Could you make sure this loop cannot throw? The rows are already committed at this point. path_referenced() starts a new transaction, so it throws when the connection is lost. RosbagCapture treats any exception from store_rosbag_files() as "nothing was stored" and deletes the new bag, and the committed rows then point to a bag that no longer exists. With the change below, the worst case is an orphaned directory, which the other comments in this file already accept.
| for (const auto & path : unique_paths) { | |
| if (path_referenced(path)) { | |
| continue; | |
| } | |
| std::error_code ec; | |
| std::filesystem::remove_all(path, ec); | |
| } | |
| for (const auto & path : unique_paths) { | |
| try { | |
| if (path_referenced(path)) { | |
| continue; | |
| } | |
| } catch (const std::exception &) { | |
| continue; // The rows are already committed, so keep the directory. | |
| } | |
| std::error_code ec; | |
| std::filesystem::remove_all(path, ec); | |
| } |
| try { | ||
| db_conn_ = std::make_unique<pqxx::connection>(base_conn_info()); | ||
| } catch (const std::exception & e) { | ||
| GTEST_SKIP() << "No PostgreSQL server reachable (set ROS2_MEDKIT_TEST_PG_CONN): " << e.what(); |
There was a problem hiding this comment.
All 98 tests in this file are skipped in CI, because CI has no PostgreSQL server. The Jazzy test log shows [ SKIPPED ] for each of them. If you want to fix this in the PR, the jazzy-test job in .github/workflows/ci.yml needs:
- a
postgresservice at the minimum version you support libpqxx-devinstalled and a build with the new option ONROS2_MEDKIT_TEST_PG_CONNset topostgresql://user:password@postgres:5432/<db>
jazzy-test runs in a container, so the service name works as the host name. After that job is in place, please change GTEST_SKIP() to FAIL() here, so a missing server is reported as a failure. With the option OFF the test target is not built, so the other jobs are not affected.
|
Hey Bartosz, Thank you for your input. I will look into your suggestions and return with fixes and/or questions. I am a little bit surprised that the tests do not pass, since I run them on my (jazzy) setup. Seems like I will have to test against humble and lyrical too since Ubuntu uses different PostgreSQL versions. Give me some time for all that and I will be back. |
|
As the commit message implies, this is not finished. I think I need some guidance though, since the changes have started getting a bit disruptive. In order to achieve the reconnection feature, many functions had to be changed to non-constant. At the same time, I presumably have guarded all disconnection exception in the fault manager node, but still a loose broken_connection is thrown somewhere. I will keep investigating tomorrow. The PostgreSQL support has now been extended to 14-18. I have also provided the equivalent postgres14 container, so that tests can now run on both the oldest and latest versions supported. Regarding Humble-Lyrical support, I have included a FetchContent declaration in the CMakeLists, but currently pqxx refuses to compile successfully. I will investigate further tomorrow. Any input would be highly appreciated! |
|
I have pushed another commit that allows for pqxx compilation through FetchContent. Reconnections should be working now. More testing might be required, since unit tests do not currently catch all potential issues. Please advice on how we can proceed. Have a nice weekend! |
|
@gstavrinos Ok, I will check it tomorrow and give you feedback. Have a nice weekend! |
|
Hi @gstavrinos I've created draft PR with potential solution to all those issues: #693 |
|
I am continuing the discussion here to avoid splitting the thread. I have studied your fixes and additions. I am angry (and ashamed) that I did not think of utilizing the I am also impressed with your attention to detail and thorough tests. I don't really anything to add or change. Impressive work! I will now use this branch to have some further testing on our setup until it is merged. Thanks for your help and effort! |
|
@gstavrinos If you can cherry pick my commit from #693 and verify that current solution works for you, I'm fine with merging it :) |
Improved SELECT result handling based on best practices, fixed typos, fixed potential password leaks from printing the database url and other minor issues
…outage - Define POSTGRES_SUPPORT only when the option is ON. The node checks it with #ifdef, so a build with the option OFF compiled the PostgreSQL code and failed to link. With the option OFF, storage_type postgres now stops the node at startup with a clear error. - Build libpqxx static and add it with EXCLUDE_FROM_ALL, so nothing of it is installed with this package. ament on Lyrical defaults BUILD_SHARED_LIBS to ON, so the option is forced OFF around it. CMake before 3.28 uses FetchContent_Populate and add_subdirectory. Remove the libpqxx-dev dependency, which the build does not use. Warnings-as-errors, clang-tidy and include-what-you-use are switched off for libpqxx only and restored afterwards; a cached -D value no longer reaches libpqxx or gets lost for the package's own targets. - Keep the FaultStorage getters const. The PostgreSQL connection is a mutable member, so the other backends and their tests stay as on main. - Start without the database when the server cannot be reached (refused, timeout, unknown host, wrong password, missing database or role). Stop only on a wrong configuration: a connection string libpq rejects, a server that accepts the connection but refuses the schema, or an existing table that lacks a column the node uses. The schema is created on the first connection that succeeds. The startup near-miss trim and HEALED reclassification are skipped while the server is unreachable, and snapshot capture ids are seeded on the first capture. - A node that cannot start logs the reason and exits with code 1. It no longer aborts with a core dump. - Bound the wait on an unreachable server. connect_timeout defaults to 2 s, tcp_user_timeout to 5000 ms and keepalives_idle to 5 s unless database_url sets them (PGCONNECT_TIMEOUT also sets connect_timeout); a libpq service keeps the values of its service file. After a failed round, requests fail at once for 5 s, then each round makes one attempt. The backoff also starts when the retries of a transaction run out, and a lost connection is retried without an extra sleep. - Never log or return the connection string. Error text from the server is logged and returned with the password from database_url or PGPASSWORD replaced by ***. The node logs host, port, database and user, or the service name; an explicit empty host or port is shown as given. - ClearFault clears the storage before it asks the correlation engine, so a clear the storage refuses leaves muted symptoms and clusters as they were. - Remove the early returns in the node constructor, which left the node running with no services after a storage error. - When the database is unreachable, services with an error field answer success=false and "Fault storage unavailable: ...". - Catch storage errors in RosbagCapture at the end of the post-fault recording and in the auto-cleanup on clear. Both run where an exception stops the process. - The time-based confirmation UPDATE uses the same last_failed_ns > 0 condition as its SELECT. - Docs: fix the empty database_url default that broke the Sphinx build. Describe the build option, libpq-dev, PostgreSQL 14 as the oldest server, one database per fault manager, what stops the node at startup and what does not, and the timeouts. - CI: a postgres job per distro (humble, jazzy, lyrical) builds the fault manager with -DPOSTGRES_SUPPORT=ON and runs its tests against a postgres:14 service. The other jobs keep the option OFF. - Fix the lint_cmake finding in the flag regex, a deprecated libpqxx call in the tests, and comments that described the SQLite backend. - The auto-confirm visibility test checks the confirmation window with the two node timestamps on the event: the event time and the fault's last occurrence. It used a clock in the test process, and a step of the system clock inside the window failed it. - Fix the compiler and clang-tidy warnings in the package: a useless cast on Humble, rclcpp::spin_some deprecated on Lyrical, and missing special members on a test helper. - The background-capture test waits until the graph carries the topic type. The constructor subscribes only to a topic whose type it can resolve, and a publisher reaches the graph some time after create_publisher returns.
Two of the three suites in the lifecycle-handler test initialised rclcpp and left it initialised. The suite that ran last therefore reached exit() with a valid default context, whose destructor calls Context::shutdown() after exit() has destroyed the thread_local state that call reads. The binary passed all its tests and then died with SIGSEGV. Both suites now shut the context down in TearDownTestSuite, as the first suite in the file already did.
|
I have cherry-picked your commits. Give me some time to test and I will be back. (Probably next week) |
Pull Request
Summary
As discussed in #649, this is an early implementation of the PostgreSQL integration. Keep in mind that currently the code does not compile because the testing suite is not included in this PR.
Issue
Link the related issue (required):
Type
Testing
Tests are yet to be implemented, so the code remains in an early stage, completely untested even for basic functionality
Checklist
TODOs (based on your checklist, will tick the list as improvements come along)
As this is still a WIP, feel free to offer suggestions, recommendations or problems you might think of.