Refactor Tasks to use builders. Do not replicate startup DDL. #1594

lmwnshn · 2021-05-27T06:08:53Z

Heading

Refactor Tasks to use builders. Do not replicate startup DDL.

Description

Fix #1576 per current understanding, we may decide in the future to add startup SQL that does get replicated, but right now everything is internal tables. Also create everything in the default noisepage database instead of in database 0 aka INVALID_DB_OID.

Mainly going from constructors like this

    task_manager->AddTask(std::make_unique<task::TaskDML>(
        catalog::INVALID_DATABASE_OID, "SELECT * FROM noisepage_forecast_frequencies",
        std::make_unique<optimizer::TrivialCostModel>(), std::move(params), std::move(param_types), freq_check, nullptr,
        settings, false, true, std::nullopt, common::ManagedPointer(&sync)));

to this

    task_manager->AddTask(task::TaskDML::Builder()
                              .SetDatabaseOid(task_manager->GetDatabaseOid())
                              .SetQueryText("SELECT * FROM noisepage_forecast_frequencies")
                              .SetFuture(common::ManagedPointer(&sync))
                              .SetParameters(std::move(params))
                              .SetParameterTypes(std::move(param_types))
                              .SetTupleFn(std::move(freq_check))
                              .SetShouldSkipQueryCache(true)
                              .SetExecutionSettings(settings)
                              .Build());

Fell down a rabbit hole of making it easier to specify additional properties for tasks in the future.

Refactor TaskDDL and TaskDML to use builders.
Track default database OID in catalog, expose via GetDefaultDatabaseOid.
Refuse INVALID_DB_OID to force callers to do the right thing, i.e., explicitly specify what database they are interacting with.
Change TryLoadStartupDDL to fail loudly instead of silently.
Disable replication of TryLoadStartupDDL.
Add TransactionPolicy arg to QueryExecUtil::BeginTransaction.
Change a const unique_ptr<> & to a ManagedPointer.
Allow SetReplicationPolicy(DISABLE) if DurabilityPolicy is also DISABLE.
Nitpick at function docstrings in QueryExecUtil.
Question: what should DurabilityPolicy be for internal table tasks?
Found bug, see DurabilityPolicy::DISABLE doesn't seem to work. #1595.

Remaining tasks

Seeing if it passes CI. I am surprised replication tests did not fail sooner.

- Refactor TaskDDL and TaskDML to use builders. - Track default database OID in catalog, expose via GetDefaultDatabaseOid. - Refuse INVALID_DB_OID to force callers to do the right thing, i.e., explicitly specify what database they are interacting with. - Change TryLoadStartupDDL to fail loudly instead of silently. - Disable replication of TryLoadStartupDDL.

- Add TransactionPolicy arg to QueryExecUtil::BeginTransaction. - Change a const unique_ptr<> & to a ManagedPointer. - Allow SetReplicationPolicy(DISABLE) if DurabilityPolicy is also DISABLE. - Nitpick at function docstrings in QueryExecUtil. - Question: what should DurabilityPolicy be for internal table tasks? - Found bug, see cmu-db#1595.

noisepage-checks · 2021-05-27T20:55:19Z

Performance Boost!

Nice job! This PR has increased the throughput of the system.

Could not find any performance results to compare for this commit.

noisepage-checks · 2021-05-28T23:28:02Z

Performance Boost!

Nice job! This PR has increased the throughput of the system.

Could not find any performance results to compare for this commit.

noisepage-checks · 2021-06-04T20:46:29Z

Performance Boost!

Nice job! This PR has increased the throughput of the system.

Could not find any performance results to compare for this commit.

noisepage-checks · 2021-06-04T22:03:56Z

Major Decrease in Performance

STOP: this PR has a major negative performance impact

tps (%change)	benchmark_type	wal_device	details
-0.74%	tpcc	RAM disk	Details master tps=22192.31, commit tps=22027.57, query_mode=extended, benchmark_type=tpcc, scale_factor=32.0000, terminals=32, client_time=60, weights={'Payment': 43, 'Delivery': 4, 'NewOrder': 45, 'StockLevel': 4, 'OrderStatus': 4}, wal_device=RAM disk, max_connection_threads=32
-2.9%	tpcc	None	Details master tps=29640.72, commit tps=28780.82, query_mode=extended, benchmark_type=tpcc, scale_factor=32.0000, terminals=32, client_time=60, weights={'Payment': 43, 'Delivery': 4, 'NewOrder': 45, 'StockLevel': 4, 'OrderStatus': 4}, wal_device=None, max_connection_threads=32
-2.15%	tpcc	HDD	Details master tps=21889.82, commit tps=21419.65, query_mode=extended, benchmark_type=tpcc, scale_factor=32.0000, terminals=32, client_time=60, weights={'Payment': 43, 'Delivery': 4, 'NewOrder': 45, 'StockLevel': 4, 'OrderStatus': 4}, wal_device=HDD, max_connection_threads=32
-1.45%	tatp	RAM disk	Details master tps=6430.86, commit tps=6337.63, query_mode=extended, benchmark_type=tatp, scale_factor=1.0000, terminals=16, client_time=60, weights={'GetAccessData': 35, 'UpdateLocation': 14, 'GetNewDestination': 10, 'GetSubscriberData': 35, 'DeleteCallForwarding': 2, 'InsertCallForwarding': 2, 'UpdateSubscriberData': 2}, wal_device=RAM disk, max_connection_threads=32
-7.7%	tatp	None	Details master tps=7417.62, commit tps=6846.65, query_mode=extended, benchmark_type=tatp, scale_factor=1.0000, terminals=16, client_time=60, weights={'GetAccessData': 35, 'UpdateLocation': 14, 'GetNewDestination': 10, 'GetSubscriberData': 35, 'DeleteCallForwarding': 2, 'InsertCallForwarding': 2, 'UpdateSubscriberData': 2}, wal_device=None, max_connection_threads=32
1.25%	tatp	HDD	Details master tps=6302.21, commit tps=6381.28, query_mode=extended, benchmark_type=tatp, scale_factor=1.0000, terminals=16, client_time=60, weights={'GetAccessData': 35, 'UpdateLocation': 14, 'GetNewDestination': 10, 'GetSubscriberData': 35, 'DeleteCallForwarding': 2, 'InsertCallForwarding': 2, 'UpdateSubscriberData': 2}, wal_device=HDD, max_connection_threads=32

turingcompl33t

LGTM. Some interesting C++ required to implement a builder when using inheritance-based runtime polymorphism!

src/include/task/task.h

src/main/db_main.cpp

src/self_driving/planning/pilot_util.cpp

src/task/task.cpp

linmagit

The refactoring looks great! Appreciate it, @lmwnshn! Just some small clarifications and comments. The main thing that I'm wondering though, is that have you tried running the internal query trace recording and the pilot with your refactor? Since we don't have tests for them in CI yet, the only way to make sure that they're not broken is to run them locally.

src/main/db_main.cpp

src/self_driving/planning/pilot_util.cpp

src/self_driving/planning/planning_context.cpp

src/util/self_driving_recording_util.cpp

Push txn_policy into the metrics manager.

noisepage-checks · 2021-06-08T00:08:42Z

Major Decrease in Performance

STOP: this PR has a major negative performance impact

tps (%change)	benchmark_type	wal_device	details
-2.11%	tpcc	RAM disk	Details master tps=22501.62, commit tps=22027.57, query_mode=extended, benchmark_type=tpcc, scale_factor=32.0000, terminals=32, client_time=60, weights={'Payment': 43, 'Delivery': 4, 'NewOrder': 45, 'StockLevel': 4, 'OrderStatus': 4}, wal_device=RAM disk, max_connection_threads=32
-0.71%	tpcc	None	Details master tps=28985.3, commit tps=28780.82, query_mode=extended, benchmark_type=tpcc, scale_factor=32.0000, terminals=32, client_time=60, weights={'Payment': 43, 'Delivery': 4, 'NewOrder': 45, 'StockLevel': 4, 'OrderStatus': 4}, wal_device=None, max_connection_threads=32
-1.24%	tpcc	HDD	Details master tps=21689.03, commit tps=21419.65, query_mode=extended, benchmark_type=tpcc, scale_factor=32.0000, terminals=32, client_time=60, weights={'Payment': 43, 'Delivery': 4, 'NewOrder': 45, 'StockLevel': 4, 'OrderStatus': 4}, wal_device=HDD, max_connection_threads=32
-0.78%	tatp	RAM disk	Details master tps=6387.57, commit tps=6337.63, query_mode=extended, benchmark_type=tatp, scale_factor=1.0000, terminals=16, client_time=60, weights={'GetAccessData': 35, 'UpdateLocation': 14, 'GetNewDestination': 10, 'GetSubscriberData': 35, 'DeleteCallForwarding': 2, 'InsertCallForwarding': 2, 'UpdateSubscriberData': 2}, wal_device=RAM disk, max_connection_threads=32
-6.33%	tatp	None	Details master tps=7309.67, commit tps=6846.65, query_mode=extended, benchmark_type=tatp, scale_factor=1.0000, terminals=16, client_time=60, weights={'GetAccessData': 35, 'UpdateLocation': 14, 'GetNewDestination': 10, 'GetSubscriberData': 35, 'DeleteCallForwarding': 2, 'InsertCallForwarding': 2, 'UpdateSubscriberData': 2}, wal_device=None, max_connection_threads=32
3.45%	tatp	HDD	Details master tps=6168.58, commit tps=6381.28, query_mode=extended, benchmark_type=tatp, scale_factor=1.0000, terminals=16, client_time=60, weights={'GetAccessData': 35, 'UpdateLocation': 14, 'GetNewDestination': 10, 'GetSubscriberData': 35, 'DeleteCallForwarding': 2, 'InsertCallForwarding': 2, 'UpdateSubscriberData': 2}, wal_device=HDD, max_connection_threads=32

noisepage-checks · 2021-06-08T00:35:23Z

Performance Boost!

Nice job! This PR has increased the throughput of the system.

Could not find any performance results to compare for this commit.

lmwnshn · 2021-06-08T01:51:50Z

Thanks for reviews! Going to call this blocked on #1566 as I hope to get that in.. for real.. in the near future..

Specifically, blocked on testing (though note that task manager test does work)

lmwnshn · 2021-08-10T00:40:06Z

While I would like this to get in, I guess this will not get in until we get the pilot in CI.

lmwnshn added in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. ready-for-ci Indicate that this build should be run through CI. labels May 27, 2021

lmwnshn self-assigned this May 27, 2021

lmwnshn added 5 commits May 27, 2021 03:52

clang-tidy ABI fix?

80a48aa

FORMAT.

cdd8d05

Mystery use-after-move.

45ba9c4

Didn't notice it was in a loop. Also, just use the string directly.

99f2b7f

lmwnshn added ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. and removed in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. labels Jun 4, 2021

lmwnshn added 8 commits June 4, 2021 13:10

This is a really weird clang-tidy error.

26449c9

Merge branch 'master' of github.com:cmu-db/noisepage into bonk_tasks

55de9f2

clang tidy??

9ee2376

Fix include dependencies, thanks Matt!

ff3010a

Fix forward declares.

e87eb21

Fix forward declares and includes.

a20b4bb

Change log info to log debug for startup ddl.

906b47d

Merge branch 'master' of github.com:cmu-db/noisepage into bonk_tasks

02def3f

turingcompl33t self-requested a review June 5, 2021 01:07

turingcompl33t approved these changes Jun 5, 2021

View reviewed changes

linmagit reviewed Jun 6, 2021

View reviewed changes

src/main/db_main.cpp Show resolved Hide resolved

src/self_driving/planning/pilot_util.cpp Outdated Show resolved Hide resolved

src/self_driving/planning/planning_context.cpp Show resolved Hide resolved

src/util/self_driving_recording_util.cpp Show resolved Hide resolved

lmwnshn added 4 commits June 7, 2021 17:34

Add Lin's comment re copying.

2038aec

Add lines between function defs.

8f70457

Undo accidental commit.

9599705

Add txn_policy as WriteToDB parameter.

1233a2f

Push txn_policy into the metrics manager.

mbutrovich removed the ready-for-ci Indicate that this build should be run through CI. label Jul 16, 2021

mbutrovich removed the ready-to-merge This PR is ready to be merged. Mark PRs with this. label Jul 27, 2021

lmwnshn closed this Aug 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Tasks to use builders. Do not replicate startup DDL. #1594

Refactor Tasks to use builders. Do not replicate startup DDL. #1594

lmwnshn commented May 27, 2021 •

edited

Loading

noisepage-checks bot commented May 27, 2021

noisepage-checks bot commented May 28, 2021

noisepage-checks bot commented Jun 4, 2021

noisepage-checks bot commented Jun 4, 2021

turingcompl33t left a comment

linmagit left a comment

noisepage-checks bot commented Jun 8, 2021

noisepage-checks bot commented Jun 8, 2021

lmwnshn commented Jun 8, 2021 •

edited

Loading

lmwnshn commented Aug 10, 2021

Refactor Tasks to use builders. Do not replicate startup DDL. #1594

Refactor Tasks to use builders. Do not replicate startup DDL. #1594

Conversation

lmwnshn commented May 27, 2021 • edited Loading

Heading

Description

Remaining tasks

noisepage-checks bot commented May 27, 2021

Performance Boost!

noisepage-checks bot commented May 28, 2021

Performance Boost!

noisepage-checks bot commented Jun 4, 2021

Performance Boost!

noisepage-checks bot commented Jun 4, 2021

Major Decrease in Performance

turingcompl33t left a comment

Choose a reason for hiding this comment

linmagit left a comment

Choose a reason for hiding this comment

noisepage-checks bot commented Jun 8, 2021

Major Decrease in Performance

noisepage-checks bot commented Jun 8, 2021

Performance Boost!

lmwnshn commented Jun 8, 2021 • edited Loading

lmwnshn commented Aug 10, 2021

lmwnshn commented May 27, 2021 •

edited

Loading

lmwnshn commented Jun 8, 2021 •

edited

Loading