Skip to content

Commit 8ad40bd

Browse files
fix: preserve loaded extensions when starting TP service (#754)
## What do these changes do? Fix Python Database.serve() to prepare TP service without rebuilding planner, preserving LOADed extensions. Add C++ regression coverage and Python GDS TP E2E test. ## Related issue number Fixes #750 --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent eec0ed1 commit 8ad40bd

6 files changed

Lines changed: 183 additions & 34 deletions

File tree

include/neug/main/neug_db.h

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,18 @@ class NeugDB {
276276
*/
277277
void CloseAllConnection();
278278

279+
/**
280+
* @brief Prepare an opened database for TP service without rebuilding
281+
* planner.
282+
*
283+
* This closes local AP connections, persists and refreshes the live graph
284+
* when needed, then rebuilds query runtime handles against the refreshed
285+
* graph. The planner and its metadata registry are intentionally preserved so
286+
* runtime extension registrations loaded in AP mode stay available in TP
287+
* mode.
288+
*/
289+
void PrepareForServing();
290+
279291
inline const PropertyGraph& graph() const {
280292
return snapshot_store_->CurrentSnapshot();
281293
}
@@ -306,22 +318,25 @@ class NeugDB {
306318
void initAllocators(const std::string& allocator_dir);
307319
void openGraphAndIngestWals();
308320
void ingestWals(IWalParser& parser, PropertyGraph& graph);
321+
void initPlanner();
322+
void initQueryRuntime();
309323
void initPlannerAndQueryProcessor();
310324
std::shared_ptr<Checkpoint> consumeLiveGraphAndCommitCheckpoint(
311325
CheckpointSession& checkpoint_session);
312-
313326
/**
314-
* @brief Create a checkpoint after WAL recovery and keep the DB open.
315-
*
316-
* A recovery checkpoint publishes the recovered graph, reopens the graph from
317-
* the published checkpoint so the live store owns checkpoint files, and rolls
318-
* back to the previous checkpoint if reopening fails.
319-
*
320-
* A durable checkpoint is a transaction timeline reset boundary: it always
321-
* compacts storage timestamps before dumping, and a successful checkpoint
322-
* resets last_ts_ to 0. Must not be called while a NeugDBService is running.
327+
* @brief Create a checkpoint and keep the DB open on the published graph.
328+
*
329+
* This publishes the current live graph and reopens it from the published
330+
* checkpoint so the live store owns checkpoint files. If reopening fails, the
331+
* published checkpoint is discarded and the checkpoint manager is restored to
332+
* the previous checkpoint generation; callers should treat the failure as
333+
* fatal. It is shared by recovery checkpoint and AP-to-TP service
334+
* preparation. A durable checkpoint is a transaction timeline reset boundary:
335+
* it always compacts storage timestamps before dumping, and a successful
336+
* checkpoint resets last_ts_ to 0. Must not be called while a NeugDBService
337+
* is running.
323338
*/
324-
void createCheckpointAfterRecovery();
339+
void createCheckpointAndRefreshLiveGraph();
325340

326341
/**
327342
* @brief Create a checkpoint while closing the DB.

src/main/neug_db.cc

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ bool NeugDB::Open(const NeugDBConfig& config) {
145145
if (last_ts_ > 0 && config.checkpoint_on_recovery &&
146146
config_.mode == DBMode::READ_WRITE) {
147147
LOG(INFO) << "Creating checkpoint after recovery at ts " << last_ts_;
148-
createCheckpointAfterRecovery();
148+
createCheckpointAndRefreshLiveGraph();
149149
}
150150
if (config_.mode == DBMode::READ_WRITE) {
151151
checkpoint_mgr_.CleanupRetiredCheckpoints();
@@ -212,6 +212,17 @@ void NeugDB::RemoveConnection(std::shared_ptr<Connection> conn) {
212212

213213
void NeugDB::CloseAllConnection() { connection_manager_->Close(); }
214214

215+
void NeugDB::PrepareForServing() {
216+
if (IsClosed()) {
217+
THROW_RUNTIME_ERROR("NeugDB instance is not ready for serving!");
218+
}
219+
CloseAllConnection();
220+
if (config_.mode == DBMode::READ_WRITE) {
221+
createCheckpointAndRefreshLiveGraph();
222+
}
223+
initQueryRuntime();
224+
}
225+
215226
void NeugDB::preprocessConfig() {
216227
if (config_.max_thread_num < 0) {
217228
THROW_INVALID_ARGUMENT_EXCEPTION(
@@ -334,16 +345,20 @@ void NeugDB::ingestWals(IWalParser& parser, PropertyGraph& graph) {
334345
last_ts_ = parser.last_ts();
335346
}
336347

337-
void NeugDB::initPlannerAndQueryProcessor() {
348+
void NeugDB::initPlanner() {
338349
if (config_.planner_kind == "gopt") {
339-
// Gopt planner is the default planner, so we don't need to create it.
340350
planner_ = std::make_shared<GOptPlanner>();
341351
} else {
342352
THROW_INVALID_ARGUMENT_EXCEPTION("Invalid planner kind: " +
343353
config_.planner_kind);
344354
}
345355
LOG(INFO) << "Finish initializing planner";
356+
}
346357

358+
void NeugDB::initQueryRuntime() {
359+
if (!planner_) {
360+
THROW_RUNTIME_ERROR("Planner is not initialized");
361+
}
347362
global_query_cache_ = std::make_shared<execution::GlobalQueryCache>(planner_);
348363

349364
query_processor_ = std::make_shared<QueryProcessor>(
@@ -354,6 +369,11 @@ void NeugDB::initPlannerAndQueryProcessor() {
354369
*snapshot_store_, planner_, query_processor_, config_);
355370
}
356371

372+
void NeugDB::initPlannerAndQueryProcessor() {
373+
initPlanner();
374+
initQueryRuntime();
375+
}
376+
357377
std::shared_ptr<Checkpoint> NeugDB::consumeLiveGraphAndCommitCheckpoint(
358378
CheckpointSession& checkpoint_session) {
359379
SnapshotGuard guard(*snapshot_store_);
@@ -365,7 +385,7 @@ std::shared_ptr<Checkpoint> NeugDB::consumeLiveGraphAndCommitCheckpoint(
365385
return published_checkpoint;
366386
}
367387

368-
void NeugDB::createCheckpointAfterRecovery() {
388+
void NeugDB::createCheckpointAndRefreshLiveGraph() {
369389
std::lock_guard<std::mutex> lock(mutex_);
370390
auto previous_checkpoint = checkpoint_mgr_.CurrentCheckpoint();
371391
auto checkpoint_session = CheckpointSession::Begin(checkpoint_mgr_);
@@ -403,8 +423,8 @@ void NeugDB::createCheckpointAfterRecovery() {
403423
throw;
404424
}
405425

406-
// Replacing snapshot_store_ releases the consumed recovery graph before the
407-
// retired checkpoint directory is removed.
426+
// Replacing snapshot_store_ releases the consumed graph before the retired
427+
// checkpoint directory is removed.
408428
previous_checkpoint.reset();
409429
checkpoint_mgr_.CleanupRetiredCheckpoints();
410430

tests/unittest/test_connection.cc

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,45 @@
1616
#include <gtest/gtest.h>
1717
#include <filesystem>
1818

19+
#include "neug/compiler/extension/extension_api.h"
20+
#include "neug/compiler/function/neug_call_function.h"
21+
#include "neug/compiler/main/metadata_registry.h"
22+
#include "neug/compiler/transaction/transaction.h"
1923
#include "neug/main/connection.h"
2024
#include "neug/main/neug_db.h"
2125
#include "unittest/utils.h"
2226

2327
namespace neug {
2428

2529
namespace test {
30+
namespace {
31+
32+
class PrepareForServingTestFunction : public function::NeugCallFunction {
33+
public:
34+
PrepareForServingTestFunction()
35+
: NeugCallFunction(
36+
"PREPARE_FOR_SERVING_TEST_EXTENSION", function::call_input_types{},
37+
{{"name", ::neug::DataType(::neug::DataTypeId::kVarchar)}}) {}
38+
};
39+
40+
struct PrepareForServingTestFunctionSet {
41+
static constexpr const char* name = "PREPARE_FOR_SERVING_TEST_EXTENSION";
42+
static function::function_set getFunctionSet() {
43+
function::function_set function_set;
44+
function_set.emplace_back(
45+
std::make_unique<PrepareForServingTestFunction>());
46+
return function_set;
47+
}
48+
};
49+
50+
bool HasPrepareForServingTestFunction() {
51+
return main::MetadataRegistry::getCatalog()->containsFunction(
52+
&transaction::DUMMY_TRANSACTION, PrepareForServingTestFunctionSet::name,
53+
false);
54+
}
55+
56+
} // namespace
57+
2658
class ConnectionTest : public ::testing::Test {
2759
protected:
2860
static constexpr const char* DB_DIR = "/tmp/connection_test";
@@ -183,6 +215,31 @@ TEST_F(ConnectionTest, TestExplicitReadAccessModeForCall) {
183215
<< project_res.error().ToString();
184216
}
185217

218+
TEST(ConnectionStandaloneTest, PrepareForServingPreservesLoadedExtensions) {
219+
constexpr const char* db_dir = "/tmp/prepare_for_serving_test";
220+
std::filesystem::remove_all(db_dir);
221+
222+
NeugDB db;
223+
NeugDBConfig config;
224+
config.data_dir = db_dir;
225+
config.mode = DBMode::READ_WRITE;
226+
config.checkpoint_on_close = false;
227+
db.Open(config);
228+
229+
auto planner = db.GetPlanner();
230+
231+
extension::ExtensionAPI::registerFunction<PrepareForServingTestFunctionSet>(
232+
catalog::CatalogEntryType::TABLE_FUNCTION_ENTRY);
233+
ASSERT_TRUE(HasPrepareForServingTestFunction());
234+
235+
db.PrepareForServing();
236+
EXPECT_EQ(planner, db.GetPlanner());
237+
EXPECT_TRUE(HasPrepareForServingTestFunction());
238+
239+
db.Close();
240+
std::filesystem::remove_all(db_dir);
241+
}
242+
186243
// Test Parallel Execution
187244
TEST_F(ConnectionTest, TestParallelExecutionAtomicity) {
188245
NeugDB db;

tools/python_bind/neug/database.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,12 @@ def serve(
339339
)
340340
except KeyboardInterrupt:
341341
self.stop_serving()
342+
raise
343+
except Exception:
344+
self._serving = False
345+
raise
346+
if blocking:
347+
self._serving = False
342348
return endpoint
343349

344350
def stop_serving(self):

tools/python_bind/src/py_database.cc

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ std::string PyDatabase::serve(int port, const std::string& host,
9292
int32_t thread_num, bool blocking,
9393
bool auto_compaction) {
9494
#ifdef BUILD_HTTP_SERVER
95+
std::lock_guard<std::recursive_mutex> lock(mtx_);
9596
if (!database) {
9697
THROW_RUNTIME_ERROR("Database is not initialized.");
9798
}
@@ -109,20 +110,8 @@ std::string PyDatabase::serve(int port, const std::string& host,
109110
". Must be less than or equal to database max_thread_num: " +
110111
std::to_string(database->config().max_thread_num) + ".");
111112
}
112-
/**
113-
* Attention here: We utilize the NeugDBService to start the server, based on
114-
* database. But we need to make some changes to the NeugDB to make it works
115-
* well for service mode, where concurrent queries will be processed.
116-
*
117-
* But before all, we need to close the database, dump the data to disk, and
118-
* then reload the database with VersionManager and multiple contexts. By
119-
* doing this, we make sure all changes made during AP mode is persisted.
120-
*/
121-
122-
std::lock_guard<std::recursive_mutex> lock(mtx_);
123113

124-
database->Close();
125-
database->Open(database->config());
114+
database->PrepareForServing();
126115
neug::ServiceConfig config;
127116
config.query_port = port;
128117
config.host_str = host;
@@ -135,11 +124,17 @@ std::string PyDatabase::serve(int port, const std::string& host,
135124
#endif
136125

137126
service_ = std::make_unique<neug::NeugDBService>(*database, config);
138-
if (blocking) {
139-
service_->run_and_wait_for_exit();
140-
return "";
127+
try {
128+
if (blocking) {
129+
service_->run_and_wait_for_exit();
130+
service_.reset();
131+
return "";
132+
}
133+
return service_->Start();
134+
} catch (...) {
135+
service_.reset();
136+
throw;
141137
}
142-
return service_->Start();
143138
#else
144139
THROW_RUNTIME_ERROR("HTTP server is not enabled in this build.");
145140
#endif

tools/python_bind/tests/test_gds.py

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,10 @@
2626
sys.path.append(os.path.join(os.path.dirname(__file__), "../"))
2727

2828
from conftest import ensure_result_cnt_gt_zero
29+
from conftest import wait_for_server_ready
2930

3031
from neug.database import Database
32+
from neug.session import Session
3133

3234

3335
def _shown_projected_graph_names(conn):
@@ -132,6 +134,60 @@ def test_run_cdlp(tmp_path):
132134
assert isinstance(row[1], int), "label should be an integer"
133135

134136

137+
def test_loaded_gds_extension_available_after_starting_tp_service(
138+
tmp_path, unused_tcp_port
139+
):
140+
"""LOAD gds in AP mode, then query a GDS procedure through TP service."""
141+
db_dir = tmp_path / "gds_tp_service_db"
142+
db = Database(db_path=str(db_dir), mode="w")
143+
db.load_builtin_dataset("tinysnb")
144+
conn = db.connect()
145+
served = False
146+
session = None
147+
try:
148+
conn.execute(
149+
"CALL project_graph("
150+
"'person_knows', "
151+
"['person'], "
152+
"{'[person, knows, person]': ''}"
153+
");"
154+
)
155+
conn.execute("LOAD gds;")
156+
conn.close()
157+
158+
endpoint = db.serve(
159+
port=unused_tcp_port,
160+
host="localhost",
161+
blocking=False,
162+
auto_compaction=False,
163+
)
164+
served = True
165+
wait_for_server_ready(endpoint)
166+
session = Session.open(endpoint, timeout="10s")
167+
rows = list(
168+
session.execute(
169+
"""
170+
CALL cdlp('person_knows', {concurrency: 1})
171+
YIELD node, label
172+
RETURN node.id, label;
173+
"""
174+
)
175+
)
176+
177+
assert len(rows) > 0, "TP cdlp must return at least one row"
178+
for row in rows:
179+
assert len(row) == 2, "each row should have (id, label)"
180+
assert isinstance(row[1], int), "label should be an integer"
181+
finally:
182+
if session is not None:
183+
session.close()
184+
if conn.is_open:
185+
conn.close()
186+
if served:
187+
db.stop_serving()
188+
db.close()
189+
190+
135191
@contextmanager
136192
def tinysnb_simple_connection(tmp_path):
137193
"""Open a writable DB with tinysnb loaded and a simple projected graph

0 commit comments

Comments
 (0)