allow virtual table and system table to share table numbers (#28008)

lift the requirement that virtual and physical tables have different table numbers, in one case: a virtual table and its corresponding physical table are allowed to have the same table number.

this does not affect table number assignment, so new virtual tables and system tables will still have distinct table numbers.

one potential concern is that `db.system.get` could see a virtual table id and think it's a physical system table id, because they have the same table number. This doesn't end up being a problem though because we always check if an ID is virtual before we try to look it up in the physical TableMapping.

tested as part of #27538 which includes the migration to make the table numbers match.

GitOrigin-RevId: 1558ece1048a9ea19abd1d87fdbfafc7fd424a3b

Author: ldanilek

crates/database/src/bootstrap_model/table.rs

@@ -316,12 +316,21 @@ impl<'a, RT: Runtime> TableModel<'a, RT> {
         let Some(table_number) = table_number else {
             return Ok(());
         };
-        if self
+        if let Ok(existing_virtual_table) = self
             .tx
             .virtual_table_mapping()
             .namespace(namespace)
-            .table_number_exists()(table_number)
+            .name(table_number)
         {
+            let existing_system_table = self
+                .tx
+                .virtual_system_mapping()
+                .virtual_to_system_table(&existing_virtual_table)?;
+            if existing_system_table == table {
+                // Setting physical table to have the same table number as its virtual table is
+                // allowed.
+                return Ok(());
+            }
             anyhow::bail!(ErrorMetadata::bad_request(
                 "TableConflict",
                 format!("New table {table} has IDs that conflict with existing system table")

crates/database/src/database.rs

@@ -470,6 +470,7 @@ impl<RT: Runtime> DatabaseSnapshot<RT> {
         table_mapping: TableMapping,
         table_states: OrdMap<TabletId, TableState>,
         index_registry: &IndexRegistry,
+        virtual_system_mapping: VirtualSystemMapping,
     ) -> anyhow::Result<TableRegistry> {
         let load_virtual_tables_timer = load_virtual_table_metadata_timer();
         let virtual_tables_table = table_mapping
@@ -492,6 +493,7 @@ impl<RT: Runtime> DatabaseSnapshot<RT> {
             table_states,
             persistence_snapshot.persistence().version(),
             virtual_table_mapping,
+            virtual_system_mapping,
         )?;
         Self::verify_invariants(&table_registry, index_registry)?;
         Ok(table_registry)
@@ -564,6 +566,7 @@ impl<RT: Runtime> DatabaseSnapshot<RT> {
         persistence: Arc<dyn PersistenceReader>,
         snapshot: RepeatableTimestamp,
         retention_validator: Arc<dyn RetentionValidator>,
+        virtual_system_mapping: VirtualSystemMapping,
     ) -> anyhow::Result<Self> {
         let repeatable_persistence: RepeatablePersistence =
             RepeatablePersistence::new(persistence.clone(), snapshot, retention_validator.clone());
@@ -624,6 +627,7 @@ impl<RT: Runtime> DatabaseSnapshot<RT> {
             table_mapping,
             table_states,
             &index_registry,
+            virtual_system_mapping,
         )
         .await?;
 
@@ -795,6 +799,7 @@ impl<RT: Runtime> Database<RT> {
             reader.clone(),
             snapshot_ts,
             Arc::new(follower_retention_manager.clone()),
+            virtual_system_mapping.clone(),
         )
         .await?;
         let max_ts = DatabaseSnapshot::<RT>::max_ts(&*reader).await?;

crates/database/src/table_registry.rs

@@ -30,6 +30,7 @@ use value::{
 use crate::{
     defaults::bootstrap_system_tables,
     metrics::bootstrap_table_registry_timer,
+    VirtualSystemMapping,
     VirtualTableMetadata,
     VIRTUAL_TABLES_TABLE,
 };
@@ -45,7 +46,10 @@ pub struct TableRegistry {
     table_mapping: TableMapping,
     persistence_version: PersistenceVersion,
 
+    /// Mapping from virtual table number to name.
     virtual_table_mapping: VirtualTableMapping,
+    /// Mapping from virtual table name to corresponding system table name.
+    virtual_system_mapping: VirtualSystemMapping,
 }
 
 impl TableRegistry {
@@ -57,13 +61,15 @@ impl TableRegistry {
         table_states: OrdMap<TabletId, TableState>,
         persistence_version: PersistenceVersion,
         virtual_table_mapping: VirtualTableMapping,
+        virtual_system_mapping: VirtualSystemMapping,
     ) -> anyhow::Result<Self> {
         let _timer = bootstrap_table_registry_timer();
         Ok(Self {
             table_mapping,
             tablet_states: table_states,
             persistence_version,
             virtual_table_mapping,
+            virtual_system_mapping,
         })
     }
 
@@ -107,7 +113,11 @@ impl TableRegistry {
                         if self.table_exists(metadata.namespace, &metadata.name) {
                             anyhow::bail!("Tried to create duplicate table {new_value}");
                         }
-                        self.validate_table_number(metadata.namespace, metadata.number)?;
+                        self.validate_table_number(
+                            metadata.namespace,
+                            metadata.number,
+                            &metadata.name,
+                        )?;
                     }
                     Some(TableUpdate {
                         namespace: metadata.namespace,
@@ -195,7 +205,11 @@ impl TableRegistry {
                     {
                         anyhow::bail!("Tried to create duplicate virtual table {new_value}");
                     }
-                    self.validate_table_number(metadata.namespace, metadata.number)?;
+                    self.validate_table_number(
+                        metadata.namespace,
+                        metadata.number,
+                        &metadata.name,
+                    )?;
                     virtual_table_creation =
                         Some((metadata.namespace, metadata.number, metadata.name));
                 },
@@ -215,6 +229,7 @@ impl TableRegistry {
         &self,
         namespace: TableNamespace,
         table_number: TableNumber,
+        table_name: &TableName,
     ) -> anyhow::Result<()> {
         anyhow::ensure!(
             !self
@@ -224,14 +239,25 @@ impl TableRegistry {
             "Cannot add a table with table number {table_number} since it already exists in the \
              table mapping"
         );
-        anyhow::ensure!(
-            !self
-                .virtual_table_mapping
-                .namespace(namespace)
-                .number_exists(table_number),
-            "Cannot add a table with table number {table_number} since it already exists in the \
-             virtual table mapping"
-        );
+        if let Ok(existing_virtual_table) = self
+            .virtual_table_mapping
+            .namespace(namespace)
+            .name(table_number)
+        {
+            if self
+                .virtual_system_mapping
+                .virtual_to_system_table(&existing_virtual_table)?
+                == table_name
+            {
+                // A virtual table can share a table number with its physical
+                // table.
+            } else {
+                anyhow::bail!(
+                    "Cannot add a table with table number {table_number} since it already exists \
+                     in the virtual table mapping"
+                );
+            }
+        }
         Ok(())
     }
 

crates/database/src/tests/mod.rs

@@ -122,6 +122,7 @@ use crate::{
     TestFacingModel,
     Transaction,
     UserFacingModel,
+    VirtualSystemMapping,
 };
 
 mod randomized_search_tests;
@@ -1887,6 +1888,7 @@ async fn test_index_write(rt: TestRuntime) -> anyhow::Result<()> {
         tp.reader(),
         unchecked_repeatable_ts(ts),
         retention_validator,
+        VirtualSystemMapping::default(),
     )
     .await?;
     let index_metadata = database_snapshot.index_registry().clone();

crates/database/src/virtual_tables/mod.rs

@@ -120,6 +120,22 @@ pub struct VirtualSystemMapping {
     system_to_virtual_doc_mapper: OrdMap<TableName, Arc<dyn VirtualSystemDocMapper>>,
 }
 
+impl std::fmt::Debug for VirtualSystemMapping {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("VirtualSystemMapping")
+            .field("virtual_to_system", &self.virtual_to_system)
+            .field("virtual_to_system_indexes", &self.virtual_to_system_indexes)
+            .finish()
+    }
+}
+
+impl PartialEq for VirtualSystemMapping {
+    fn eq(&self, other: &Self) -> bool {
+        self.virtual_to_system == other.virtual_to_system
+            && self.virtual_to_system_indexes == other.virtual_to_system_indexes
+    }
+}
+
 impl VirtualSystemMapping {
     pub fn add_table(
         &mut self,

crates/function_runner/src/in_memory_indexes.rs

@@ -334,6 +334,7 @@ impl<RT: Runtime> InMemoryIndexCache<RT> {
             table_states,
             persistence_snapshot.persistence().version(),
             virtual_table_mapping,
+            virtual_system_mapping(),
         )?;
         DatabaseSnapshot::<RT>::verify_invariants(&table_registry, &index_registry)?;
         let in_memory_indexes = FunctionRunnerInMemoryIndexes {

crates/model/src/file_storage/mod.rs

@@ -355,11 +355,51 @@ impl<'a, RT: Runtime> FileStorageModel<'a, RT> {
 
 #[cfg(test)]
 mod tests {
+    use std::collections::{
+        BTreeMap,
+        BTreeSet,
+    };
+
+    use common::{
+        document::ID_FIELD,
+        query::{
+            Order,
+            Query,
+        },
+    };
+    use database::{
+        query::TableFilter,
+        test_helpers::DbFixtures,
+        DeveloperQuery,
+        ImportFacingModel,
+        ResolvedQuery,
+        TableModel,
+    };
     use pb::storage::FileStorageId as FileStorageIdProto;
     use proptest::prelude::*;
+    use runtime::testing::TestRuntime;
     use sync_types::testing::assert_roundtrips;
+    use uuid::Uuid;
+    use value::{
+        sha256::Sha256,
+        val,
+        ConvexObject,
+        DeveloperDocumentId,
+        TableNamespace,
+    };
 
-    use super::FileStorageId;
+    use super::{
+        FileStorageId,
+        FileStorageModel,
+    };
+    use crate::{
+        file_storage::{
+            types::FileStorageEntry,
+            FILE_STORAGE_TABLE,
+            FILE_STORAGE_VIRTUAL_TABLE,
+        },
+        test_helpers::DbFixturesWithModel,
+    };
 
     proptest! {
         #![proptest_config(
@@ -371,4 +411,102 @@ mod tests {
             assert_roundtrips::<FileStorageId, FileStorageIdProto>(left);
         }
     }
+
+    #[convex_macro::test_runtime]
+    async fn test_file_storage_change_number_to_virtual(rt: TestRuntime) -> anyhow::Result<()> {
+        let fixtures = DbFixtures::new_with_model(&rt).await?;
+        let db = fixtures.db;
+        let mut tx = db.begin_system().await?;
+        FileStorageModel::new(&mut tx, TableNamespace::test_user())
+            .store_file(FileStorageEntry {
+                storage_id: Uuid::new_v4().into(),
+                storage_key: "abc".try_into()?,
+                sha256: Sha256::new().finalize(),
+                size: 0,
+                content_type: None,
+            })
+            .await?;
+        db.commit_with_write_source(tx, "test").await?;
+
+        let mut tx = db.begin_system().await?;
+        let virtual_table_number = tx
+            .virtual_table_mapping()
+            .namespace(TableNamespace::Global)
+            .number(&FILE_STORAGE_VIRTUAL_TABLE)?;
+        let table_id = TableModel::new(&mut tx)
+            .insert_table_for_import(
+                TableNamespace::test_user(),
+                &FILE_STORAGE_TABLE,
+                Some(virtual_table_number),
+                &BTreeSet::new(),
+            )
+            .await?;
+        let table_mapping_for_schema = tx.table_mapping().clone();
+        let mut query = ResolvedQuery::new(
+            &mut tx,
+            TableNamespace::test_user(),
+            Query::full_table_scan(FILE_STORAGE_TABLE.clone(), Order::Asc),
+        )?;
+        while let Some(doc) = query.next(&mut tx, None).await? {
+            let physical_id = doc.developer_id();
+            let virtual_id =
+                DeveloperDocumentId::new(table_id.table_number, physical_id.internal_id());
+            let mut object: BTreeMap<_, _> = doc.into_value().0.into();
+            object.insert(ID_FIELD.clone().into(), val!(virtual_id.to_string()));
+
+            ImportFacingModel::new(&mut tx)
+                .insert(
+                    table_id,
+                    &FILE_STORAGE_TABLE,
+                    ConvexObject::try_from(object)?,
+                    &table_mapping_for_schema,
+                )
+                .await?;
+        }
+        TableModel::new(&mut tx)
+            .activate_table(
+                table_id.tablet_id,
+                &FILE_STORAGE_TABLE,
+                table_id.table_number,
+                &BTreeSet::new(),
+            )
+            .await?;
+        db.commit_with_write_source(tx, "migrate_file_storage_table")
+            .await?;
+
+        let mut tx = db.begin_system().await?;
+        let mut count = 0;
+        let virtual_table_number = tx
+            .virtual_table_mapping()
+            .namespace(TableNamespace::test_user())
+            .number(&FILE_STORAGE_VIRTUAL_TABLE)?;
+        let system_table_number = tx
+            .table_mapping()
+            .namespace(TableNamespace::test_user())
+            .name_to_id()(FILE_STORAGE_TABLE.clone())?
+        .table_number;
+        assert_eq!(virtual_table_number, system_table_number);
+        let mut query = ResolvedQuery::new(
+            &mut tx,
+            TableNamespace::test_user(),
+            Query::full_table_scan(FILE_STORAGE_TABLE.clone(), Order::Asc),
+        )?;
+        while let Some(doc) = query.next(&mut tx, None).await? {
+            let physical_id = doc.developer_id();
+            let mut get_query = DeveloperQuery::new_with_version(
+                &mut tx,
+                TableNamespace::test_user(),
+                Query::get(FILE_STORAGE_VIRTUAL_TABLE.clone(), physical_id),
+                Some("1.7.0".parse()?),
+                TableFilter::ExcludePrivateSystemTables,
+            )?;
+            let Some(virtual_doc) = get_query.next(&mut tx, None).await? else {
+                anyhow::bail!("Virtual document {physical_id} not found");
+            };
+            assert_eq!(virtual_doc.id(), physical_id);
+            count += 1;
+        }
+        assert_eq!(count, 1);
+        Ok(())
+    }
 }