From eba48445745cd05ac959538e03b0a8de64cea6c9 Mon Sep 17 00:00:00 2001 From: David Lutterkort Date: Tue, 25 Mar 2025 13:12:39 -0700 Subject: [PATCH 1/4] store: Do not emit an index in block$ twice If an immutable entity type has a `block` column, we would emit the index on the block$ twice, making deployment fail --- store/postgres/src/relational/ddl_tests.rs | 80 ++++++++++++++++++++++ store/postgres/src/relational/index.rs | 30 ++++++-- 2 files changed, 103 insertions(+), 7 deletions(-) diff --git a/store/postgres/src/relational/ddl_tests.rs b/store/postgres/src/relational/ddl_tests.rs index bb1dcc67f46..53106be2b1a 100644 --- a/store/postgres/src/relational/ddl_tests.rs +++ b/store/postgres/src/relational/ddl_tests.rs @@ -352,6 +352,74 @@ fn can_copy_from() { ); } +/// Check that we do not create the index on `block$` twice. There was a bug +/// that if an immutable entity type had a `block` field and index creation +/// was postponed, we would emit the index on `block$` twice, once from +/// `Table.create_time_travel_indexes` and once through +/// `IndexList.indexes_for_table` +#[test] +fn postponed_indexes_with_block_column() { + fn index_list() -> IndexList { + // To generate this list, print the output of `layout.as_ddl(None)`, run + // that in Postgres and do `select indexdef from pg_indexes where + // schemaname = 'sgd0815'` + const INDEX_DEFS: &[&str] = &[ + "CREATE UNIQUE INDEX data_pkey ON sgd0815.data USING btree (vid)", + "CREATE UNIQUE INDEX data_id_key ON sgd0815.data USING btree (id)", + "CREATE INDEX data_block ON sgd0815.data USING btree (block$)", + "CREATE INDEX attr_1_0_data_block ON sgd0815.data USING btree (block, \"block$\")", + ]; + + let mut indexes: HashMap> = HashMap::new(); + indexes.insert( + "data".to_string(), + INDEX_DEFS + .iter() + .map(|def| CreateIndex::parse(def.to_string())) + .collect(), + ); + IndexList { indexes } + } + // Names of the two indexes we are interested in. Not the leading space + // to guard a little against overlapping names + const BLOCK_IDX: &str = " data_block"; + const ATTR_IDX: &str = " attr_1_0_data_block"; + + let layout = test_layout(BLOCK_GQL); + + // Create everything + let sql = layout.as_ddl(None).unwrap(); + assert!(sql.contains(BLOCK_IDX)); + assert!(sql.contains(ATTR_IDX)); + + // Defer attribute indexes + let sql = layout.as_ddl(Some(index_list())).unwrap(); + assert!(sql.contains(BLOCK_IDX)); + assert!(!sql.contains(ATTR_IDX)); + // This used to be duplicated + let count = sql.matches(BLOCK_IDX).count(); + assert_eq!(1, count); + + let table = layout.table(&SqlName::from("Data")).unwrap(); + let sql = table.create_postponed_indexes(vec![], false); + assert_eq!(1, sql.len()); + assert!(!sql[0].contains(BLOCK_IDX)); + assert!(sql[0].contains(ATTR_IDX)); + + let dst_nsp = Namespace::new("sgd2".to_string()).unwrap(); + let arr = index_list() + .indexes_for_table(&dst_nsp, &table.name.to_string(), &table, true, false) + .unwrap(); + assert_eq!(1, arr.len()); + assert!(!arr[0].1.contains(BLOCK_IDX)); + assert!(arr[0].1.contains(ATTR_IDX)); + + let arr = index_list() + .indexes_for_table(&dst_nsp, &table.name.to_string(), &table, false, false) + .unwrap(); + assert_eq!(0, arr.len()); +} + const THING_GQL: &str = r#" type Thing @entity { id: ID! @@ -1109,3 +1177,15 @@ on "sgd0815"."stats_3_day" using btree("volume"); create index stats_3_day_dims on "sgd0815"."stats_3_day"(group_2, group_1, timestamp); "#; + +const BLOCK_GQL: &str = r#" +type Block @entity(immutable: true) { + id: ID! + number: Int! +} + +type Data @entity(immutable: true) { + id: ID! + block: Block! +} +"#; diff --git a/store/postgres/src/relational/index.rs b/store/postgres/src/relational/index.rs index 4f72e773ee6..77e7c2c400d 100644 --- a/store/postgres/src/relational/index.rs +++ b/store/postgres/src/relational/index.rs @@ -123,7 +123,7 @@ impl Display for Expr { Expr::Column(s) => write!(f, "{s}")?, Expr::Prefix(s, _) => write!(f, "{s}")?, Expr::Vid => write!(f, "vid")?, - Expr::Block => write!(f, "block")?, + Expr::Block => write!(f, "{BLOCK_COLUMN}")?, Expr::BlockRange => write!(f, "block_range")?, Expr::BlockRangeLower => write!(f, "lower(block_range)")?, Expr::BlockRangeUpper => write!(f, "upper(block_range)")?, @@ -488,12 +488,29 @@ impl CreateIndex { && columns[1] == Expr::BlockRange } Method::Brin => false, - Method::BTree | Method::Gin => { + Method::Gin => { + // 'using gin()' columns.len() == 1 && columns[0].is_attribute() && cond.is_none() && with.is_none() } + Method::BTree => { + match columns.len() { + 1 => { + // 'using btree()' + columns[0].is_attribute() && cond.is_none() && with.is_none() + } + 2 => { + // 'using btree(, block$)' + columns[0].is_attribute() + && columns[1] == Expr::Block + && cond.is_none() + && with.is_none() + } + _ => false, + } + } Method::Unknown(_) => false, } } @@ -537,6 +554,7 @@ impl CreateIndex { None, ), dummy(false, BTree, &[Expr::BlockRangeUpper], Some(Cond::Closed)), + dummy(false, BTree, &[Expr::Block], None), ] }; } @@ -630,7 +648,7 @@ impl CreateIndex { } pub fn fields_exist_in_dest<'a>(&self, dest_table: &'a Table) -> bool { - fn column_exists<'a>(it: &mut impl Iterator, column_name: &String) -> bool { + fn column_exists<'a>(it: &mut impl Iterator, column_name: &str) -> bool { it.any(|c| *c == *column_name) } @@ -667,9 +685,7 @@ impl CreateIndex { } Expr::Vid => (), Expr::Block => { - if !column_exists(cols, &"block".to_string()) { - return false; - } + return dest_table.immutable; } Expr::Unknown(expression) => { if some_column_contained( @@ -776,7 +792,7 @@ impl IndexList { // First we check if the fields do exist in the destination subgraph. // In case of grafting that is not given. if ci.fields_exist_in_dest(dest_table) - // Then we check if the index is one of the default indexes not based on + // Then we check if the index is one of the default indexes not based on // the attributes. Those will be created anyway and we should skip them. && !ci.is_default_non_attr_index() // Then ID based indexes in the immutable tables are also created initially From b27cc722e5ac0c0bcf5ef4e0bae7ade35e27203f Mon Sep 17 00:00:00 2001 From: David Lutterkort Date: Tue, 25 Mar 2025 17:10:06 -0700 Subject: [PATCH 2/4] store: IndexList.indexes_for_table: split concurrent and if_not_exists At the end of copying, we do not want to create indexes concurrently, but we do want the creation to be idempotent, i.e., have a 'if not exists' clause --- store/postgres/src/copy.rs | 1 + store/postgres/src/relational/ddl.rs | 9 ++++++++- store/postgres/src/relational/ddl_tests.rs | 18 ++++++++++++++++-- store/postgres/src/relational/index.rs | 7 ++++--- 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/store/postgres/src/copy.rs b/store/postgres/src/copy.rs index d82bc33e4a8..d92064c7f5c 100644 --- a/store/postgres/src/copy.rs +++ b/store/postgres/src/copy.rs @@ -731,6 +731,7 @@ impl Connection { &table.dst, true, false, + true, )?; for (_, sql) in arr { diff --git a/store/postgres/src/relational/ddl.rs b/store/postgres/src/relational/ddl.rs index 980bca2b9fd..e85281a5899 100644 --- a/store/postgres/src/relational/ddl.rs +++ b/store/postgres/src/relational/ddl.rs @@ -408,7 +408,14 @@ impl Table { if index_def.is_some() && ENV_VARS.postpone_attribute_index_creation { let arr = index_def .unwrap() - .indexes_for_table(&self.nsp, &self.name.to_string(), &self, false, false) + .indexes_for_table( + &self.nsp, + &self.name.to_string(), + &self, + false, + false, + false, + ) .map_err(|_| fmt::Error)?; for (_, sql) in arr { writeln!(out, "{};", sql).expect("properly formated index statements") diff --git a/store/postgres/src/relational/ddl_tests.rs b/store/postgres/src/relational/ddl_tests.rs index 53106be2b1a..c9e44854e8f 100644 --- a/store/postgres/src/relational/ddl_tests.rs +++ b/store/postgres/src/relational/ddl_tests.rs @@ -408,14 +408,28 @@ fn postponed_indexes_with_block_column() { let dst_nsp = Namespace::new("sgd2".to_string()).unwrap(); let arr = index_list() - .indexes_for_table(&dst_nsp, &table.name.to_string(), &table, true, false) + .indexes_for_table( + &dst_nsp, + &table.name.to_string(), + &table, + true, + false, + false, + ) .unwrap(); assert_eq!(1, arr.len()); assert!(!arr[0].1.contains(BLOCK_IDX)); assert!(arr[0].1.contains(ATTR_IDX)); let arr = index_list() - .indexes_for_table(&dst_nsp, &table.name.to_string(), &table, false, false) + .indexes_for_table( + &dst_nsp, + &table.name.to_string(), + &table, + false, + false, + false, + ) .unwrap(); assert_eq!(0, arr.len()); } diff --git a/store/postgres/src/relational/index.rs b/store/postgres/src/relational/index.rs index 77e7c2c400d..5776bf8f01f 100644 --- a/store/postgres/src/relational/index.rs +++ b/store/postgres/src/relational/index.rs @@ -784,7 +784,8 @@ impl IndexList { table_name: &String, dest_table: &Table, postponed: bool, - concurrent_if_not_exist: bool, + concurrent: bool, + if_not_exists: bool, ) -> Result, String)>, Error> { let mut arr = vec![]; if let Some(vec) = self.indexes.get(table_name) { @@ -805,7 +806,7 @@ impl IndexList { { if let Ok(sql) = ci .with_nsp(namespace.to_string())? - .to_sql(concurrent_if_not_exist, concurrent_if_not_exist) + .to_sql(concurrent, if_not_exists) { arr.push((ci.name(), sql)) } @@ -829,7 +830,7 @@ impl IndexList { let namespace = &layout.catalog.site.namespace; for table in layout.tables.values() { for (ind_name, create_query) in - self.indexes_for_table(namespace, &table.name.to_string(), table, true, true)? + self.indexes_for_table(namespace, &table.name.to_string(), table, true, true, true)? { if let Some(index_name) = ind_name { let table_name = table.name.clone(); From 6aa489ccb7f5d78e9ba35dd670844390fd32626f Mon Sep 17 00:00:00 2001 From: David Lutterkort Date: Wed, 26 Mar 2025 12:14:00 -0700 Subject: [PATCH 3/4] store: Make test postponed_indexes_with_block_column more specific Test that the create index statements do/do not contain 'if not exists' --- store/postgres/src/relational/ddl_tests.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/store/postgres/src/relational/ddl_tests.rs b/store/postgres/src/relational/ddl_tests.rs index c9e44854e8f..b15a40cecfb 100644 --- a/store/postgres/src/relational/ddl_tests.rs +++ b/store/postgres/src/relational/ddl_tests.rs @@ -380,6 +380,15 @@ fn postponed_indexes_with_block_column() { ); IndexList { indexes } } + + fn cr(index: &str) -> String { + format!("create index{}", index) + } + + fn cre(index: &str) -> String { + format!("create index if not exists{}", index) + } + // Names of the two indexes we are interested in. Not the leading space // to guard a little against overlapping names const BLOCK_IDX: &str = " data_block"; @@ -389,12 +398,12 @@ fn postponed_indexes_with_block_column() { // Create everything let sql = layout.as_ddl(None).unwrap(); - assert!(sql.contains(BLOCK_IDX)); - assert!(sql.contains(ATTR_IDX)); + assert!(sql.contains(&cr(BLOCK_IDX))); + assert!(sql.contains(&cr(ATTR_IDX))); // Defer attribute indexes let sql = layout.as_ddl(Some(index_list())).unwrap(); - assert!(sql.contains(BLOCK_IDX)); + assert!(sql.contains(&cr(BLOCK_IDX))); assert!(!sql.contains(ATTR_IDX)); // This used to be duplicated let count = sql.matches(BLOCK_IDX).count(); @@ -404,7 +413,7 @@ fn postponed_indexes_with_block_column() { let sql = table.create_postponed_indexes(vec![], false); assert_eq!(1, sql.len()); assert!(!sql[0].contains(BLOCK_IDX)); - assert!(sql[0].contains(ATTR_IDX)); + assert!(sql[0].contains(&cre(ATTR_IDX))); let dst_nsp = Namespace::new("sgd2".to_string()).unwrap(); let arr = index_list() @@ -419,7 +428,7 @@ fn postponed_indexes_with_block_column() { .unwrap(); assert_eq!(1, arr.len()); assert!(!arr[0].1.contains(BLOCK_IDX)); - assert!(arr[0].1.contains(ATTR_IDX)); + assert!(arr[0].1.contains(&cr(ATTR_IDX))); let arr = index_list() .indexes_for_table( From 0e2ee0ae0b3f011c918187fd409c2c309671516f Mon Sep 17 00:00:00 2001 From: David Lutterkort Date: Wed, 26 Mar 2025 12:14:53 -0700 Subject: [PATCH 4/4] store: Address logic error in CreateIndex.fields_exist_in_dest Do not short-circuit checking other columns in the check for Block --- store/postgres/src/relational/index.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/store/postgres/src/relational/index.rs b/store/postgres/src/relational/index.rs index 5776bf8f01f..efa82e901f0 100644 --- a/store/postgres/src/relational/index.rs +++ b/store/postgres/src/relational/index.rs @@ -685,7 +685,9 @@ impl CreateIndex { } Expr::Vid => (), Expr::Block => { - return dest_table.immutable; + if !dest_table.immutable { + return false; + } } Expr::Unknown(expression) => { if some_column_contained(