Skip to content

Conversation

@johnnyjoy
Copy link

@johnnyjoy johnnyjoy commented Dec 17, 2025

Adds PostgreSQL as an optional backend for the rollback system, complementing the existing SQLite3 implementation. The rollback backend is configurable per-world via rollback_backend in world.mt, defaulting to sqlite3 when unset.

Changes

  • New PostgreSQL rollback backend (RollbackMgrPostgreSQL) with feature parity to SQLite3
  • Strict configuration: requires explicit pgsql_rollback_connection when PostgreSQL is selected (no fallback to map backend)
  • Shared base class (RollbackMgr) to eliminate code duplication between backends
  • Refactored initialization: extracted rollback manager creation from Server::init() into Server::createRollbackManager()
  • PostgreSQL 9.5+ requirement: enforces minimum version
  • Documentation updates: doc/world_format.md and builtin/settingtypes.txt updated

Configuration

Set in world.mt:

  • rollback_backend = postgresql (or sqlite3)
  • pgsql_rollback_connection = "host=... dbname=..." (required when using PostgreSQL)

Development Process

This PR was developed with assistance from Cursor and LLM tools for code generation and refactoring. All changes have been reviewed against project coding standards, and feedback from maintainers (sfan5, SmallJoker) has been incorporated throughout the development process.

@Zughy Zughy added @ Startup / Config / Util Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand labels Dec 17, 2025
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

The rollback code is a bit neglected, but surprisingly many people still use it and I guess adding another backend is okay.

@sfan5 sfan5 self-requested a review December 19, 2025 19:36
@sfan5 sfan5 added Roadmap: supported by core dev PR not adhering to the roadmap, yet some core dev decided to take care of it and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand labels Dec 19, 2025
@sfan5 sfan5 self-requested a review December 27, 2025 21:27
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Dec 28, 2025
@SmallJoker SmallJoker self-requested a review December 28, 2025 20:24
@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Dec 29, 2025
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Dec 31, 2025
@johnnyjoy johnnyjoy force-pushed the pr/rollback-postgresql branch from 058d001 to ddcb9a3 Compare January 1, 2026 18:40
@sfan5 sfan5 requested a review from SmallJoker January 7, 2026 16:04
johnnyjoy and others added 11 commits January 7, 2026 12:21
Extract the large cache loading blocks from RollbackManagerPostgreSQL
constructor into loadActorCache() and loadNodeCache() helper methods.
This reduces the constructor from ~33 lines to 5 lines, addressing
sfan5's feedback about large code blocks.
Co-authored-by: SmallJoker <[email protected]>
Improve error handling in Server::createRollbackManager
- Throw exception if world.mt cannot be read, matching openModStorageDatabase pattern
- Remove unused backend_source variable and simplify logging
- Simplify backend selection using world_mt.getNoEx() instead of if-else block

Enhance error messages and help output
- Add Server::getRollbackBackends() to dynamically list available backends
- Use dynamic backend list in error message for unsupported backends
- Add rollback backends to print_help() output in main.cpp
- Remove redundant USE_POSTGRESQL=OFF specific error message

Improve destructor safety and consistency
- Move db != nullptr check from SQLite destructor to flush() method
- Move initialized() check from PostgreSQL destructor to flush() method
- Ensures consistent safety checks across both backends

Restore spacing alignment for readability
- Add space padding after sqlite3_bind_* function names
- Matches existing pattern in rollback_sqlite3.cpp

Rename RollbackManager to RollbackMgr for line length compliance
- Use RollbackMgr abbreviation following existing codebase patterns
- Rename RollbackManager → RollbackMgr
- Rename RollbackManagerSQLite3 → RollbackMgrSQLite3
- Rename RollbackManagerPostgreSQL → RollbackMgrPostgreSQL
- Break long function signatures to ensure all lines are under 95 characters
…ng bool

The original code would throw std::out_of_range when substr() was called
with npos, making the fallback code that bound 0,0,0 unreachable dead
code. This change makes parseNodemetaLocation() throw
std::invalid_argument on invalid format, matching the original behavior
and eliminating the dead code.

Changes:
Change parseNodemetaLocation() signature from bool to void
Throw std::invalid_argument exceptions instead of returning false
Remove unreachable fallback blocks in SQLite and PostgreSQL backends
Add #include <stdexcept> to rollback.cpp

This addresses SmallJoker's feedback.
Address SmallJoker's request for unit tests to verify that
parseNodemetaLocation() correctly throws std::invalid_argument on
invalid format.

Tests cover:
Valid format parsing (basic, negative, zero coordinates)
Invalid format exceptions (missing prefix, missing commas, empty string)
@johnnyjoy johnnyjoy force-pushed the pr/rollback-postgresql branch from 5a0716d to 3cb71ba Compare January 7, 2026 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Action / change needed Code still needs changes (PR) / more information requested (Issues) Feature ✨ PRs that add or enhance a feature Roadmap: supported by core dev PR not adhering to the roadmap, yet some core dev decided to take care of it @ Startup / Config / Util

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants