Skip to content

Commit dcf7cdb

Browse files
committed
adapter: remove apply_item_update optimization
When applying an item `StateUpdate`, we had an optimization that would skip parsing the `create_sql` if it hadn't change, and reuse the old `CatalogEntry` instead. This produces the wrong outcome if the `extra_versions` change without the SQL changing: Any version updates are not reflected in the new `CatalogEntry`. To fix this, this commit removes the optimization, under the assumption that DDL is sufficiently rare that we can live without it. If that turns out not the be the case, we can keep the optimization and refine the conditions under which SQL parsing is performed, though that seems brittle.
1 parent 252f011 commit dcf7cdb

File tree

1 file changed

+23
-25
lines changed

1 file changed

+23
-25
lines changed

src/adapter/src/catalog/apply.rs

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,33 +1014,31 @@ impl CatalogState {
10141014
item: name.clone(),
10151015
};
10161016
let entry = match retractions.items.remove(&key) {
1017-
Some(mut retraction) => {
1017+
Some(retraction) => {
10181018
assert_eq!(retraction.id, item.id);
1019-
// We only reparse the SQL if it's changed. Otherwise, we use the existing
1020-
// item. This is a performance optimization and not needed for correctness.
1021-
// This makes it difficult to use the `UpdateFrom` trait, but the structure
1022-
// is still the same as the trait.
1023-
if retraction.create_sql() != create_sql {
1024-
let item = self
1025-
.deserialize_item(
1026-
global_id,
1027-
&create_sql,
1028-
&extra_versions,
1029-
local_expression_cache,
1030-
Some(retraction.item),
1031-
)
1032-
.unwrap_or_else(|e| {
1033-
panic!("{e:?}: invalid persisted SQL: {create_sql}")
1034-
});
1035-
retraction.item = item;
1036-
}
1037-
retraction.id = id;
1038-
retraction.oid = oid;
1039-
retraction.name = name;
1040-
retraction.owner_id = owner_id;
1041-
retraction.privileges = PrivilegeMap::from_mz_acl_items(privileges);
10421019

1043-
retraction
1020+
let item = self
1021+
.deserialize_item(
1022+
global_id,
1023+
&create_sql,
1024+
&extra_versions,
1025+
local_expression_cache,
1026+
Some(retraction.item),
1027+
)
1028+
.unwrap_or_else(|e| {
1029+
panic!("{e:?}: invalid persisted SQL: {create_sql}")
1030+
});
1031+
1032+
CatalogEntry {
1033+
item,
1034+
id,
1035+
oid,
1036+
name,
1037+
owner_id,
1038+
privileges: PrivilegeMap::from_mz_acl_items(privileges),
1039+
referenced_by: retraction.referenced_by,
1040+
used_by: retraction.used_by,
1041+
}
10441042
}
10451043
None => {
10461044
let catalog_item = self

0 commit comments

Comments
 (0)