Update TableSchema & system tables to resemble ABI V9 #1697

kazimuth · 2024-09-11T16:12:24Z

Description of Changes

This gets rid of most of the usages of RawModuleDefV8 in core. It replaces it with ModuleDef, which means that the new validation path is used consistently throughout the codebase. It also refactors the system tables & TableSchema types to match the new ModuleDef, the update functionality to use the new auto-migration functionality based on ModuleDef, and does a little bit of other cleanup.

The major user-visible changes from this PR are:

Some modules that used to be allowed may be rejected by the new, intensive validation rules.
The describe endpoint now returns a json-serialized RawModuleDefV9 rather than RawModuleDefV8.

The major logical changes are:

ConstraintSchema and IndexSchema (and their corresponding system tables) no longer store redundant information. Constraints are completely separate from indices; to determine if an index is unique, look up a constraint with its columns. (This is not quite true in mut_tx.rs and the table crate, see my discussion with tyler below, but this is now true throughout more of the codebase.)
Also note that IndexSchema stores a ColList, to keep track of field ordering, whereas ConstraintSchema stores a ColSet, which discards ordering, but can be cheaply constructed from a ColList.
The Constraints type is now constructed on-demand from TableSchema, rather than being stored directly in TableSchema. This means we have a single source of truth for the data in Constraints and are less likely to muck it up. Constraints is still used in the vm and sql modules.

API and ABI breaking changes

ABI break in the system tables, API break at the describe endpoint.

Expected complexity level and risk

4, this is a system table update and touches things all over core.

Testing

Updated all tests and added some. See also #1737

crates/core/src/db/datastore/system_tables.rs

crates/primitives/src/ids.rs

crates/core/src/db/relational_db.rs

crates/core/src/sql/compiler.rs

crates/lib/src/relation.rs

crates/schema/src/schema.rs

crates/core/src/db/datastore/system_tables.rs

crates/core/src/db/update.rs

crates/core/src/db/datastore/traits.rs

Get compiling, fix birdbrained API System table rewrite done More progress Get rid of useless generic args WIP module_host.rs errorless Lots more compilation successes Finish update.rs A bunch of easy fixes . CORE COMPILING BABY Core and planner tests compiling Everything but CLI compiling

…nt with old names

kazimuth · 2024-09-24T18:03:47Z

There's now a fix for the internal tests in private, so this is all ready. Together with #1370, update is broken but that shouldn't be a problem.

kazimuth · 2024-09-24T18:18:22Z

crates/core/src/db/update.rs

-use enum_as_inner::EnumAsInner;
-use itertools::Itertools;
-use similar::{Algorithm, TextDiff};
+use crate::execution_context::ExecutionContext;


This file was completely rewritten so should probably not be reviewed in diff view.

kazimuth · 2024-09-24T18:20:13Z

crates/core/src/vm.rs

@@ -541,39 +540,6 @@ impl<'db, 'tx> DbProgram<'db, 'tx> {
        }
    }

-    fn _create_table(&mut self, table: RawTableDefV8) -> Result<Code, ErrorVm> {


This code was dead and now out-of-date so I buried it.

crates/core/src/db/datastore/locking_tx_datastore/datastore.rs

crates/core/src/db/datastore/system_tables.rs

cloutiertyler

I scanned the whole PR. I left a few small nits. AFAICT, this looks really good. Very nice job here @kazimuth.

I am approving this PR from a traits.rs perspective and no major objection to the rest, but would probably be good to get another approval as well.

kim

Looks good.

Error propagation needs another pass.

crates/core/src/db/relational_db.rs

kim · 2024-09-25T06:38:52Z

crates/core/src/db/datastore/system_tables.rs

+/// It's fine to call this on system table types, because `validate_system_table` checks that
+/// they are `ProductType`s.
+///
+/// TODO: this performs some unnecessary allocation. We may want to reimplement the conversions manually for


crates/core/src/host/host_controller.rs

kim · 2024-09-25T07:15:13Z

crates/core/src/host/wasm_common/module_host_actor.rs

+                log::warn!("Database update failed: {} @ {}", e, stdb.address());
+                self.system_logger().warn(&format!("Database update failed: {e}"));


Seen this so many times that I feel like there should be a tee logger which can log to more than one sink.

Would be a good idea to just log everything that goes through the system loggers yeah. I'll make that a separate issue tho

crates/core/src/db/update.rs

kazimuth added the abi-break A PR that makes an ABI breaking change label Sep 11, 2024

Base automatically changed from jgilles/algebraic_type_for_generate to master September 11, 2024 20:44

kazimuth force-pushed the jgilles/newschema_real branch from 5c7c851 to 0b0849b Compare September 11, 2024 21:06

kazimuth commented Sep 11, 2024

View reviewed changes

crates/core/src/db/datastore/system_tables.rs Show resolved Hide resolved

kazimuth commented Sep 11, 2024

View reviewed changes

crates/core/src/db/datastore/system_tables.rs Show resolved Hide resolved

kazimuth commented Sep 11, 2024

View reviewed changes

crates/primitives/src/ids.rs Outdated Show resolved Hide resolved

kazimuth force-pushed the jgilles/newschema_real branch 2 times, most recently from b771dd4 to 893b425 Compare September 13, 2024 18:25

kazimuth requested a review from cloutiertyler as a code owner September 13, 2024 21:01

bfops added the release-0.12 label Sep 16, 2024

kazimuth force-pushed the jgilles/newschema_real branch from e5415c3 to 804a774 Compare September 17, 2024 17:04

mamcx self-requested a review September 18, 2024 14:38

mamcx reviewed Sep 18, 2024

View reviewed changes

crates/core/src/db/relational_db.rs Outdated Show resolved Hide resolved

crates/core/src/sql/compiler.rs Show resolved Hide resolved

crates/lib/src/relation.rs Outdated Show resolved Hide resolved

crates/schema/src/schema.rs Show resolved Hide resolved

kazimuth force-pushed the jgilles/newschema_real branch from 0e0d9a3 to d7d8c04 Compare September 18, 2024 19:38

mamcx reviewed Sep 20, 2024

View reviewed changes

crates/core/src/db/datastore/system_tables.rs Outdated Show resolved Hide resolved

mamcx reviewed Sep 20, 2024

View reviewed changes

crates/core/src/db/datastore/system_tables.rs Show resolved Hide resolved

mamcx reviewed Sep 20, 2024

View reviewed changes

crates/core/src/db/datastore/system_tables.rs Outdated Show resolved Hide resolved

kazimuth mentioned this pull request Sep 20, 2024

WIP: updating Schema and system tables #1662

Closed

2 tasks

kazimuth commented Sep 20, 2024

View reviewed changes

crates/core/src/db/update.rs Show resolved Hide resolved

kazimuth force-pushed the jgilles/newschema_real branch 2 times, most recently from 2553709 to 63cda1f Compare September 23, 2024 18:12

cloutiertyler reviewed Sep 24, 2024

View reviewed changes

crates/core/src/db/datastore/traits.rs Show resolved Hide resolved

kazimuth added 8 commits September 24, 2024 12:32

Get cli compiling

2f437e9

ALL CRATES COMPILING WOOOOO

27ebe20

Fix 19 tests

13edb20

Fix system vars

ec07284

Fix bootstrapping, rework auto-generated v9 names to be more consiste…

5645710

…nt with old names

Fix 20 more tests

1d6c95f

Fix some more tests

66cce1c

kazimuth added 9 commits September 24, 2024 12:32

Placate clippy

bbd49bc

Fix schema tests

a84ee0e

Fix an automatic migration issue

14c9afd

Fix some smoketests

772c31a

You need to commit your database updates -.-

1ae50f2

Better documentation and associated consts for system IDs

23dc8e9

Address review comments

abfee97

Update some TODO comments

57fbb5f

Handle generated schedule IDs correctly

b371e6d

kazimuth force-pushed the jgilles/newschema_real branch from 5f16f02 to b371e6d Compare September 24, 2024 16:32

kim self-requested a review September 24, 2024 17:43

kazimuth commented Sep 24, 2024

View reviewed changes

kazimuth added 2 commits September 24, 2024 14:31

Update some comments

b4604c0

Tidy up a bit more

5a34374

cloutiertyler reviewed Sep 24, 2024

View reviewed changes

crates/core/src/db/datastore/locking_tx_datastore/datastore.rs Outdated Show resolved Hide resolved

cloutiertyler reviewed Sep 24, 2024

View reviewed changes

crates/core/src/db/datastore/system_tables.rs Outdated Show resolved Hide resolved

cloutiertyler approved these changes Sep 24, 2024

View reviewed changes

coolreader18 mentioned this pull request Sep 25, 2024

Rust module API rework #1660

Merged

kim approved these changes Sep 25, 2024

View reviewed changes

kazimuth added 2 commits September 25, 2024 15:31

Address reviews

5148eea

Final review comments addressed.

6200368

kazimuth enabled auto-merge September 25, 2024 19:49

kazimuth added this pull request to the merge queue Sep 25, 2024

Merged via the queue into master with commit c32f297 Sep 25, 2024
7 of 8 checks passed

lcodes pushed a commit that referenced this pull request Sep 30, 2024

Update TableSchema & system tables to resemble ABI V9 (#1697)

794b1b8

lcodes pushed a commit that referenced this pull request Oct 1, 2024

Update TableSchema & system tables to resemble ABI V9 (#1697)

9b7421a

lcodes pushed a commit that referenced this pull request Oct 1, 2024

Update TableSchema & system tables to resemble ABI V9 (#1697)

8c40d1e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update TableSchema & system tables to resemble ABI V9 #1697

Update TableSchema & system tables to resemble ABI V9 #1697

kazimuth commented Sep 11, 2024 •

edited

Loading

kazimuth commented Sep 24, 2024 •

edited

Loading

kazimuth Sep 24, 2024

kazimuth Sep 24, 2024

cloutiertyler left a comment

kim left a comment

kim Sep 25, 2024

kim Sep 25, 2024

kazimuth Sep 25, 2024

		log::warn!("Database update failed: {} @ {}", e, stdb.address());
		self.system_logger().warn(&format!("Database update failed: {e}"));

Update TableSchema & system tables to resemble ABI V9 #1697

Update TableSchema & system tables to resemble ABI V9 #1697

Conversation

kazimuth commented Sep 11, 2024 • edited Loading

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

kazimuth commented Sep 24, 2024 • edited Loading

kazimuth Sep 24, 2024

Choose a reason for hiding this comment

kazimuth Sep 24, 2024

Choose a reason for hiding this comment

cloutiertyler left a comment

Choose a reason for hiding this comment

kim left a comment

Choose a reason for hiding this comment

kim Sep 25, 2024

Choose a reason for hiding this comment

kim Sep 25, 2024

Choose a reason for hiding this comment

kazimuth Sep 25, 2024

Choose a reason for hiding this comment

kazimuth commented Sep 11, 2024 •

edited

Loading

kazimuth commented Sep 24, 2024 •

edited

Loading