From 3c6400673d76e76494aab9c5bba8202c4b6b2413 Mon Sep 17 00:00:00 2001 From: Jordan Hrycaj Date: Wed, 4 Sep 2024 13:48:38 +0000 Subject: [PATCH] Coredb fix kvt save only fringe condition (#2592) * Cosmetics, spelling, etc. * Aristo: make sure that a save cycle always commits even when empty why: If `Kvt` is tied to the `Aristo` DB save cycle, then this save cycle must also be committed if there is no data to save for `Aristo`. Otherwise this will lead to excessive core memory use with some fringe condition where Eth headers (or blocks) are downloaded while syncing and not really stored on disk. * CoreDb: Correct persistent save mode why: Saving `Kvt` first is seen as a harbinger (or canary) for `Aristo` as both run in sync. If `Kvt` succeeds saving first, so must be `Aristo` next. Other than this is a defect. --- nimbus/db/aristo/aristo_delta.nim | 17 +++++++++++------ nimbus/db/aristo/aristo_init/rocks_db.nim | 6 ++---- nimbus/db/core_db/base.nim | 18 ++++++------------ nimbus/db/kvt/kvt_init/rocks_db/rdb_desc.nim | 2 +- 4 files changed, 20 insertions(+), 23 deletions(-) diff --git a/nimbus/db/aristo/aristo_delta.nim b/nimbus/db/aristo/aristo_delta.nim index f408a394c..6bcb78c29 100644 --- a/nimbus/db/aristo/aristo_delta.nim +++ b/nimbus/db/aristo/aristo_delta.nim @@ -13,16 +13,12 @@ ## import - std/[strutils, tables], - chronicles, + std/tables, eth/common, results, ./aristo_delta/[delta_merge, delta_reverse], ./aristo_desc/desc_backend, - "."/[aristo_desc, aristo_get, aristo_layers, aristo_utils] - -logScope: - topics = "aristo-delta" + "."/[aristo_desc, aristo_layers] # ------------------------------------------------------------------------------ # Public functions, save to backend @@ -55,6 +51,15 @@ proc deltaPersistent*( # Blind or missing filter if db.balancer.isNil: + # Add a blind storage frame. This will do no harm if `Aristo` runs + # standalone. Yet it is needed if a `Kvt` is tied to `Aristo` and has + # triggered a save cyle already which is to be completed here. + # + # There is no need to add a blind frame on any error return. If there + # is a `Kvt` tied to `Aristo`, then it must somehow run in sync and an + # error occuring here must have been detected earlier when (implicitely) + # registering `Kvt`. So that error should be considered a defect. + ? be.putEndFn(? be.putBegFn()) return ok() # Make sure that the argument `db` is at the centre so the backend is in diff --git a/nimbus/db/aristo/aristo_init/rocks_db.nim b/nimbus/db/aristo/aristo_init/rocks_db.nim index 15c51c054..d5179c3e1 100644 --- a/nimbus/db/aristo/aristo_init/rocks_db.nim +++ b/nimbus/db/aristo/aristo_init/rocks_db.nim @@ -80,7 +80,7 @@ proc getVtxFn(db: RdbBackendRef): GetVtxFn = # Fetch serialised data record let vtx = db.rdb.getVtx(rvid).valueOr: when extraTraceMessages: - trace logTxt "getVtxFn() failed", vid, error=error[0], info=error[1] + trace logTxt "getVtxFn() failed", rvid, error=error[0], info=error[1] return err(error[0]) if vtx.isValid: @@ -95,7 +95,7 @@ proc getKeyFn(db: RdbBackendRef): GetKeyFn = # Fetch serialised data record let key = db.rdb.getKey(rvid).valueOr: when extraTraceMessages: - trace logTxt "getKeyFn: failed", vid, error=error[0], info=error[1] + trace logTxt "getKeyFn: failed", rvid, error=error[0], info=error[1] return err(error[0]) if key.isValid: @@ -207,8 +207,6 @@ proc putEndFn(db: RdbBackendRef): PutEndFn = case hdl.error.pfx: of VtxPfx, KeyPfx: trace logTxt "putEndFn: vtx/key failed", pfx=hdl.error.pfx, vid=hdl.error.vid, error=hdl.error.code - of FilPfx: trace logTxt "putEndFn: filter failed", - pfx=FilPfx, qid=hdl.error.qid, error=hdl.error.code of AdmPfx: trace logTxt "putEndFn: admin failed", pfx=AdmPfx, aid=hdl.error.aid.uint64, error=hdl.error.code of Oops: trace logTxt "putEndFn: oops", diff --git a/nimbus/db/core_db/base.nim b/nimbus/db/core_db/base.nim index 249278797..32b5d8342 100644 --- a/nimbus/db/core_db/base.nim +++ b/nimbus/db/core_db/base.nim @@ -166,8 +166,7 @@ proc `$$`*(e: CoreDbError): string = proc persistent*( db: CoreDbRef; blockNumber: BlockNumber; - ): CoreDbRc[void] - {.discardable.} = + ): CoreDbRc[void] = ## This function stored cached data from the default context (see `ctx()` ## below) to the persistent database. ## @@ -187,16 +186,11 @@ proc persistent*( else: result = err(rc.error.toError $api) break body - block: - let rc = CoreDbAccRef(db.ctx).call(persist, db.ctx.mpt, blockNumber) - if rc.isOk: - discard - elif CoreDbAccRef(db.ctx).call(level, db.ctx.mpt) != 0: - result = err(rc.error.toError($api, TxPending)) - break body - else: - result = err(rc.error.toError $api) - break body + # Having reached here `Aristo` must not fail as both `Kvt` and `Aristo` + # are kept in sync. So if there is a legit fail condition it mist be + # caught in the previous clause. + CoreDbAccRef(db.ctx).call(persist, db.ctx.mpt, blockNumber).isOkOr: + raiseAssert $api & ": " & $error result = ok() db.ifTrackNewApi: debug logTxt, api, elapsed, blockNumber, result diff --git a/nimbus/db/kvt/kvt_init/rocks_db/rdb_desc.nim b/nimbus/db/kvt/kvt_init/rocks_db/rdb_desc.nim index 7d5e43075..60b2ea2ef 100644 --- a/nimbus/db/kvt/kvt_init/rocks_db/rdb_desc.nim +++ b/nimbus/db/kvt/kvt_init/rocks_db/rdb_desc.nim @@ -24,7 +24,7 @@ type session*: WriteBatchRef ## For batched `put()` basePath*: string ## Database directory - delayedPersist*: KvtDbRef ## Enable next proggyback write session + delayedPersist*: KvtDbRef ## Enable next piggyback write session KvtCFs* = enum ## Column family symbols/handles and names used on the database