core: Fix schema checks in database updates, again

[kim]

It turns out that the changes introduced in #734 do not result in more reliable detection of incompatible schema updates. This is because the datastructures involved can be converted into each other, but that conversion is not bijective.

Fix this by manually adjusting the schema of the existing table to be comparable to the proposed table.

Also log details about a schema mismatch to the user-retrievable database log, in unified diff format.

Description of Changes

Please describe your change, mention any related tickets, and so on here.

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Expected complexity level and risk

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

[mamcx]

It looks to me like the only change necessary is to sort the columns here, all the other stuff is done in TableSchema::from_def, so I propose that instead, we create TableSchema:sort_columns(mut self) that also recompute the row_type and keep the rest the same:

fn compute_row_type(columns: &[ColumnSchema]) -> ProductType {
        ProductType::new(
            columns
                .iter()
                .map(|c| ProductTypeElement {
                    name: Some(c.col_name.clone()),
                    algebraic_type: c.col_type.clone(),
                })
                .collect(),
        )
    }

    pub fn sort_columns(&mut self) {
        self.columns.sort_by_key(|x| x.col_pos);
        self.row_type = Self::compute_row_type(&self.columns);
    }

then is only:

        if let Some(known_schema) = known_tables.remove(proposed_table_name) {
            let mut known_schema =
                TableSchema::from_def(known_schema.table_id, TableDef::from(known_schema.into_owned()));
            known_schema.sort_columns();

            let proposed_schema = TableSchema::from_def(known_schema.table_id, proposed_schema_def);

            if proposed_schema != known_schema {

[kim]

That is not correct, from_def also adds additional “generated” definitions and doesn’t correctly remove them.

Please feel free to fix the datatypes instead.

[mamcx]

Do you mean that in from_def the generated get duplicates from the ones supplied by TableDef?

Anyway, if is the case then approve this PR and later fix it

[kim]

Yes, from_def assumes that the given TableDef cannot know about those generated values (because the module cannot know about them). However, converting an existing TableSchema into a TableDef will leave the generated ones in place. So if we turn that into a TableSchema again, there will be duplicates.

I added this to the docstring.

It is hard to tell which conversions should be legal -- my expectation would be that TableSchema -> TableDef should "forget" about anything a TableDef coming from a module would not contain. I would also expect that TableSchema::new ensures the same ordering as TableSchema::from_def.

It is also odd that all collections are Vecs, when semantically they should be sets. I suppose that's because what is stored in the system tables is canonical ultimately.

One approach could be to make validated correct all of those potential pitfalls, and return a newtype. So we know we're comparing Validated(TableSchema) == Validated(Tableschema).

[mamcx]

LGTM. Fix the issues with round-trips is for a later PR