Advance ripple.app.rdb #3965

undertome · 2021-10-23T03:03:50Z

High Level Overview of Change

This pull request refactors the ripple.app.rdb module (the Relational Database Interface) with the goal of making it easier to use, read, and maintain, and also features updated documentation.

Context of Change

This refactor improves Doxygen comments, documentation, naming, and the organization of the files in the rdb directory by increasing modularity.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Test Plan

Since there aren't any substantial changes to the logic, only unit tests were conducted.

Future Tasks

This is likely the first of several iterative improvements to this module.

cjcobb23 · 2021-10-26T16:08:24Z

@manojsdoshi Let's run CI on this after the review has wrapped up but before merge, because a lot of this Postgres code is not unit tested. Need to make sure we didn't break anything.

ximinez

Looks good. I left a couple of minor comments, but I won't need to re-review once they're addressed.

src/ripple/app/rdb/backend/detail/impl/Shard.cpp

src/ripple/app/rdb/impl/Vacuum.cpp

scottschurr

Sorry I took so long getting through this code review.

The changes look good. I pointed out a number of things that, in my opinion, could be improved. But I think most of the issues I've pointed out were already in the various files.

I'm open to discussing any of the suggestions I made.

In total this looks like a substantial improvement on the current situation. Thanks.

scottschurr · 2021-11-09T00:58:53Z

src/ripple/app/ledger/Ledger.cpp

@@ -927,8 +927,7 @@ saveValidatedLedger(
        return true;
    }

-    auto res = dynamic_cast<RelationalDBInterfaceSqlite*>(
-                   &app.getRelationalDBInterface())
+    auto res = dynamic_cast<SQLiteDatabase*>(&app.getRelationalDatabase())


You have a couple of choices here. A failed dynamic_cast returns a nullptr. But you're not checking for a nullptr, and indirecting through a nullptr will immediately crash the server.

A dynamic_cast is expensive. So if you're not going to take advantage of the returned nullptr it makes more sense to use a static_cast, which is cheaper and will also work in the successful case. A failed static_cast will certainly blow up as spectacularly in the failing case, but not in as predictable a fashion.

Personally, I'd be inclined to stick with the dynamic_cast, but check the return value for nullptr before proceeding. However if there's some way that you can guarantee to me that an SQLiteDatabase is the only possible return value forever and for always, then the static_cast could make sense. But in that case you should leave a comment about why the static_cast is always safe.

scottschurr · 2021-11-09T01:00:30Z

src/ripple/app/ledger/Ledger.cpp

+    auto nodestoreHashes =
+        dynamic_cast<PostgresDatabase*>(&app.getRelationalDatabase())
+            ->getTxHashes(ledger.info().seq);


See earlier comment about checking the return value of a dynamic_cast for nullptr.

scottschurr · 2021-11-09T01:11:17Z

src/ripple/app/main/Application.cpp

@@ -1021,8 +1021,7 @@ class ApplicationImp : public Application, public BasicApp
        ledgerCleaner_->stop();
        if (reportingETL_)
            reportingETL_->stop();
-        if (auto pg = dynamic_cast<RelationalDBInterfacePostgres*>(
-                &*mRelationalDBInterface))
+        if (auto pg = dynamic_cast<PostgresDatabase*>(&*mRelationalDatabase))


FWIW, here's an example of properly checking the return value of a dynamic_cast.

scottschurr · 2021-11-09T01:28:54Z

src/ripple/app/main/DBInit.h

@@ -72,12 +72,22 @@ inline constexpr std::array<char const*, 5> LgrDBInit{
 // Transaction database holds transactions and public keys
 inline constexpr auto TxDBName{"transaction.db"};

-inline constexpr std::array TxDBPragma
+// Omitting the explicit template parameters
+// seemed to caused undefined behaviour


I'd change this comment to "In C++17 omitting the explicit template parameters caused a crash."

scottschurr · 2021-11-09T01:30:08Z

src/ripple/app/main/DBInit.h

@@ -117,12 +127,22 @@ inline constexpr std::array<char const*, 8> TxDBInit{
 // The Ledger Meta database maps ledger hashes to shard indexes
 inline constexpr auto LgrMetaDBName{"ledger_meta.db"};

-inline constexpr std::array LgrMetaDBPragma
+// Omitting the explicit template parameters
+// seemed to caused undefined behaviour


See the above comment.

scottschurr · 2021-11-19T19:32:56Z

src/ripple/rpc/handlers/AccountTx.cpp

@@ -305,16 +305,16 @@ doAccountTxHelp(RPC::Context& context, AccountTxArgs const& args)
    {
        if (args.forward)
        {
-            auto [tx, marker] = dynamic_cast<RelationalDBInterfaceSqlite*>(
-                                    &context.app.getRelationalDBInterface())
+            auto [tx, marker] = dynamic_cast<SQLiteDatabase*>(


See earlier comment about checking the return value of a dynamic_cast for nullptr.

scottschurr · 2021-11-19T19:33:07Z

src/ripple/rpc/handlers/AccountTx.cpp

                                    ->oldestAccountTxPage(options);
            result.transactions = tx;
            result.marker = marker;
        }
        else
        {
-            auto [tx, marker] = dynamic_cast<RelationalDBInterfaceSqlite*>(
-                                    &context.app.getRelationalDBInterface())
+            auto [tx, marker] = dynamic_cast<SQLiteDatabase*>(


See earlier comment about checking the return value of a dynamic_cast for nullptr.

scottschurr · 2021-11-19T19:33:30Z

src/ripple/rpc/handlers/GetCounts.cpp

@@ -75,22 +75,19 @@ getCountsJson(Application& app, int minObjectCount)

    if (!app.config().reporting() && app.config().useTxTables())
    {
-        auto dbKB = dynamic_cast<RelationalDBInterfaceSqlite*>(
-                        &app.getRelationalDBInterface())
+        auto dbKB = dynamic_cast<SQLiteDatabase*>(&app.getRelationalDatabase())


See earlier comment about checking the return value of a dynamic_cast for nullptr.

scottschurr · 2021-11-19T19:33:39Z

src/ripple/rpc/handlers/GetCounts.cpp

                        ->getKBUsedAll();

        if (dbKB > 0)
            ret[jss::dbKBTotal] = dbKB;

-        dbKB = dynamic_cast<RelationalDBInterfaceSqlite*>(
-                   &app.getRelationalDBInterface())
+        dbKB = dynamic_cast<SQLiteDatabase*>(&app.getRelationalDatabase())


See earlier comment about checking the return value of a dynamic_cast for nullptr.

Nice job getting rid of the dynamic_cast altogether!

scottschurr · 2021-11-19T19:34:05Z

src/ripple/rpc/handlers/GetCounts.cpp

                   ->getKBUsedLedger();

        if (dbKB > 0)
            ret[jss::dbKBLedger] = dbKB;

-        dbKB = dynamic_cast<RelationalDBInterfaceSqlite*>(
-                   &app.getRelationalDBInterface())
+        dbKB = dynamic_cast<SQLiteDatabase*>(&app.getRelationalDatabase())


See earlier comment about checking the return value of a dynamic_cast for nullptr.

scottschurr

👍 Changes look good to me. You should consider removing the unused method either in this pull request or a future pull request. But that change is not worth holding up this pull request.

undertome · 2021-12-21T17:54:30Z

Rebased to HEAD of develop

ximinez · 2022-03-03T22:44:42Z

src/ripple/app/ledger/Ledger.cpp

+    auto const res = dynamic_cast<SQLiteDatabase*>(&app.getRelationalDatabase())
+                         ->saveValidatedLedger(ledger, current);
+
+    if (!res)
+        Throw<std::runtime_error>("Failed to get relational database");


This doesn't look right. You're checking the result of saveValidatedLedger, which would not justify a throw with this message. I think what you're going for is:

auto const db = dynamic_cast<SQLiteDatabase*>(&app.getRelationalDatabase()); if (!db) Throw<std::runtime_error>("Failed to get relational database"); auto const res = db->saveValidatedLedger(ledger, current);

Good catch. Done

ximinez

Nicely done!

This commit addresses minor bugs introduced with commit 6faaa91: - The number of threads used by the database engine was incorrectly clamped to the lower possible value, such that the database was effectively operating in single threaded mode. - The number of requests to extract at once was so high that it could result in increased latency. The bundle size is now limited to 4 and can be adjusted by a new configuration option `rq_bundle` in the `[node_db]` stanza. This is an advanced tunable and adjusting it should not be needed.

Several hard-coded parameters control the behavior of the ledger acquisition engine. The values of many of these parameters where set by intuition and have complex and non-intuitive interactions with each other and other parts of the code. An earlier commit attempted to adjust several of these parameters to improve syncing performance; initial testing was promising but a number of operators reported experiencing syncing and stability issues with their servers. As a result, this commit reverts parts of commit 1823506. This commit further adjusts some tunables so as to increase the aggressiveness of the ledger acquisition engine.

failing online delete healch check.

Fixes XRPLF#3996.

* "A path is considered invalid if and only if it enters and exits an address node through trust lines where No Ripple has been enabled for that address." (https://xrpl.org/rippling.html#specifics) * When loading trust lines for an account "Alice" which was reached via a trust line that has the No Ripple flag set on Alice's side, do not use or cache any of Alice's trust lines which have the No Ripple flag set on Alice's side. For typical "end-user" accounts, this will return no trust lines.

* Also remove an unused variable that's giving VS a problem

undertome added Tech Debt Non-urgent improvements Documentation README changes, code comments, etc. labels Oct 23, 2021

undertome requested review from miguelportilla and ximinez October 23, 2021 03:03

undertome assigned miguelportilla and ximinez Oct 26, 2021

undertome force-pushed the relational-database branch 12 times, most recently from 40f18f1 to c887f80 Compare November 4, 2021 14:57

undertome force-pushed the relational-database branch from c887f80 to 683dafd Compare November 4, 2021 18:57

undertome requested a review from scottschurr November 5, 2021 18:33

undertome assigned scottschurr Nov 5, 2021

XRPLF deleted a comment Nov 8, 2021

ximinez approved these changes Nov 11, 2021

View reviewed changes

src/ripple/app/rdb/backend/detail/impl/Shard.cpp Outdated Show resolved Hide resolved

src/ripple/app/rdb/backend/detail/impl/Shard.cpp Outdated Show resolved Hide resolved

src/ripple/app/rdb/impl/Vacuum.cpp Outdated Show resolved Hide resolved

ximinez approved these changes Nov 12, 2021

View reviewed changes

scottschurr removed the request for review from miguelportilla November 18, 2021 18:35

scottschurr unassigned miguelportilla Nov 18, 2021

scottschurr reviewed Nov 19, 2021

View reviewed changes

undertome force-pushed the relational-database branch from 8622163 to d268183 Compare November 24, 2021 18:23

scottschurr approved these changes Nov 30, 2021

View reviewed changes

undertome force-pushed the relational-database branch from d268183 to 78f1539 Compare November 30, 2021 20:57

undertome force-pushed the relational-database branch from 78f1539 to edc0572 Compare December 21, 2021 17:53

ximinez requested changes Mar 3, 2022

View reviewed changes

ximinez approved these changes Mar 28, 2022

View reviewed changes

nbougalis and others added 11 commits May 2, 2022 22:16

Check triggers cmake error unnecessarily

6c2ed50

Fix test failures with newer mscv compilers (related to string aliasing)

558c3e3

Instead of abending, wait until server stabilizes after

6fc5629

failing online delete healch check.

Reduce log level of "Unable to find ledger" message:

9519c61

Fixes XRPLF#3996.

Track total trustlines, avoid duplications

691f938

* Also remove an unused variable that's giving VS a problem

Amendment: ExpandedSignerList

bf577fc

Address failures in DatabaseDownloader unittest

34efb7c

Set version to 1.9.1-b1

1f2f50f

undertome force-pushed the relational-database branch 2 times, most recently from ba27cde to 5dd35e5 Compare May 4, 2022 20:58

Advance ripple.app.rdb

6760ad4

undertome force-pushed the relational-database branch from 5dd35e5 to 6760ad4 Compare May 4, 2022 21:41

This was referenced May 10, 2022

Propose 1.9.1-b1 #4158

Closed

Proposed 1.9.1-b1 #4161

Merged

manojsdoshi closed this in #4161 May 11, 2022

intelliot referenced this pull request Aug 28, 2023

Advance ripple.app.rdb

dac080f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advance ripple.app.rdb #3965

Advance ripple.app.rdb #3965

undertome commented Oct 23, 2021 •

edited

Loading

cjcobb23 commented Oct 26, 2021

ximinez left a comment

scottschurr left a comment

scottschurr Nov 9, 2021

undertome Nov 22, 2021

scottschurr Nov 9, 2021

undertome Nov 22, 2021

scottschurr Nov 9, 2021

scottschurr Nov 9, 2021

undertome Nov 22, 2021

scottschurr Nov 9, 2021

undertome Nov 22, 2021

scottschurr Nov 19, 2021

undertome Nov 24, 2021

scottschurr Nov 19, 2021

undertome Nov 24, 2021

scottschurr Nov 19, 2021

undertome Nov 24, 2021

scottschurr Nov 19, 2021

undertome Nov 24, 2021

scottschurr Nov 30, 2021

scottschurr Nov 19, 2021

undertome Nov 24, 2021

scottschurr left a comment

undertome commented Dec 21, 2021

ximinez Mar 3, 2022

undertome Mar 28, 2022

ximinez left a comment

Advance ripple.app.rdb #3965

Advance ripple.app.rdb #3965

Conversation

undertome commented Oct 23, 2021 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

Test Plan

Future Tasks

cjcobb23 commented Oct 26, 2021

ximinez left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

undertome commented Dec 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

undertome commented Oct 23, 2021 •

edited

Loading