Graphman copy indexing improvements #5425

zorancv · 2024-05-17T12:33:46Z

Addresses the #5140 issue. The tests are still pending.

lutter · 2024-06-06T00:29:47Z

store/postgres/src/relational/ddl_tests.rs

@@ -155,31 +155,31 @@ fn test_manual_index_creation_ddl() {
 #[test]
 fn generate_ddl() {
 let layout = test_layout(THING_GQL);
- let sql = layout.as_ddl().expect("Failed to generate DDL");
+ let sql = layout.as_ddl(false).expect("Failed to generate DDL");


At least for one of these, there should also be a test that passes true to demonstrate which indexes are created in that case.

lutter · 2024-06-06T00:32:35Z

store/postgres/src/deployment_store.rs

@@ -180,6 +180,7 @@ impl DeploymentStore {
 graft_base: Option<Arc<Layout>>,
 replace: bool,
 on_sync: OnSync,
+ is_copy_op: bool,


It might be clearer to use a separate enum here rather than passing a boolean flag, maybe something like enum IndexMode { Immediate, Delay }. That enum could then also encapsulate some of the logic around when to create an index and when not, like mode.should_create(column, index_method) -> bool

lutter · 2024-06-06T00:33:45Z

store/postgres/src/relational/ddl.rs

+ writeln!(out)
+ }
+
+ fn colums_to_index(&self) -> impl Iterator<Item = &Column> {


typo in columns_to_index

lutter · 2024-06-08T01:58:16Z

store/postgres/src/copy.rs

+ // the copy/graft operations.
+ // First recreate the indexes that existed in the original subgraph.
+ let conn = self.conn.deref_mut();
+ let namespace = self.dst.site.namespace.as_str().to_string();


This is very minor, but why not pass the Namespace around and only convert that to astring when it's absolutely needed?

lutter · 2024-06-08T02:00:02Z

store/postgres/src/copy.rs

@@ -828,7 +871,7 @@ impl Connection {
 /// lower(v1.block_range) => v2.vid > v1.vid` and we can therefore stop
 /// the copying of each table as soon as we hit `max_vid = max { v.vid |
 /// lower(v.block_range) <= target_block.number }`.
- pub fn copy_data(&mut self) -> Result<Status, StoreError> {
+ pub fn copy_data(&mut self, index_list: IndexList) -> Result<Status, StoreError> {


It would be good to have a comment here that says that index_list is the list of indexes that currently exist on the source

lutter · 2024-06-08T02:29:12Z

store/postgres/src/relational/index.rs

+ }
+ }
+ false
+ }


Isn't this just columns.iter().any(|c| c == column_name)? I am all for leaving this a helper for readability, though

lutter · 2024-06-08T02:29:35Z

store/postgres/src/relational/index.rs

+ false
+ }
+
+ fn some_column_contained(expresion: &String, columns: &Vec<String>) -> bool {


Typo in expression

lutter · 2024-06-08T02:31:48Z

store/postgres/src/relational/index.rs

+ .columns
+ .iter()
+ .map(|i| i.name.to_string())
+ .collect::<Vec<String>>();


This is very minor here, and can stay like this, since this code isn't run very often, but in general it is better to avoid allocations like that; if this code was called more often, the helpers above should take a Table and iterate over its columns without allocating

lutter · 2024-06-08T02:34:44Z

store/postgres/src/relational/index.rs

+ .collect::<Vec<String>>();
+
+ match self {
+ CreateIndex::Unknown { defn: _ } => (),


Is it correct that we return true for an index that we couldn't parse? It would be clearer if this arm had a return in it.

lutter · 2024-06-08T02:35:21Z

store/postgres/src/relational/prune.rs

- dst.as_ddl(schema, catalog, &mut query)?;
+ // In case of pruning we don't do delayed creation of indexes,
+ // as the asumption is that there is not that much data inserted.
+ dst.as_ddl(schema, catalog, None, &mut query)?;


zorancv marked this pull request as draft May 17, 2024 12:34

zorancv changed the base branch from master to zoran/graphman-copy-index May 17, 2024 12:36

zorancv force-pushed the zoran/graphman-copy-same branch 2 times, most recently from 4450622 to a656ea2 Compare May 17, 2024 14:11

zorancv force-pushed the zoran/graphman-copy-index branch from 6287b01 to 1c4f291 Compare May 17, 2024 14:13

zorancv changed the base branch from zoran/graphman-copy-index to zoran/graphman-copy-init May 17, 2024 14:14

zorancv changed the base branch from zoran/graphman-copy-init to zoran/graphman-copy-index May 17, 2024 14:14

zorancv changed the base branch from zoran/graphman-copy-index to master May 17, 2024 14:18

zorancv changed the base branch from master to zoran/graphman-copy-index May 17, 2024 14:19

zorancv changed the base branch from zoran/graphman-copy-index to tiago/receipts-in-mappings May 17, 2024 14:20

zorancv changed the base branch from tiago/receipts-in-mappings to zoran/graphman-copy-index May 17, 2024 14:20

zorancv changed the base branch from zoran/graphman-copy-index to zoran/graphman-copy-init May 17, 2024 14:20

zorancv changed the base branch from zoran/graphman-copy-init to master May 17, 2024 14:21

zorancv force-pushed the zoran/graphman-copy-same branch 4 times, most recently from cbd2732 to 2204c6e Compare May 22, 2024 16:26

zorancv changed the title ~~Grahman copy indexing improvements~~ Graphman copy indexing improvements May 23, 2024

zorancv force-pushed the zoran/graphman-copy-same branch 10 times, most recently from 844d9e2 to 35dbb91 Compare May 29, 2024 09:38

zorancv changed the base branch from master to zoran/graphman-copy-only May 29, 2024 11:35

zorancv changed the base branch from zoran/graphman-copy-only to master May 29, 2024 13:11

zorancv force-pushed the zoran/graphman-copy-same branch 6 times, most recently from 4df57e5 to e4b8b6c Compare May 30, 2024 14:38

zorancv added 16 commits May 30, 2024 21:22

disable creation of attribute indexes for copy operation

cd978cb

create postponed indexes

1c0d570

Guard index postponed creation with env var

9917f29

cleanup, comments

2fa3f04

tests for postponed index creation

3f9c8eb

recreate indexes if they aren't valid

eea0596

pick raw SQL

29e4b39

wip

210f264

check if the colums still exist in graft case

c523a4c

fix check if in

cadb353

wip graft recreation of indexes for changed entity fields

197ba6e

fix graft test

dffaa5f

remove not needed parameter

8e8b523

fix for id indexes on immutable tables

7bdeba4

fix recreation of indexes

c85ab0d

added fields in graft

b53a561

zorancv force-pushed the zoran/graphman-copy-same branch 4 times, most recently from 4881b62 to 668b7c4 Compare May 31, 2024 13:54

fix grafting

042641c

zorancv force-pushed the zoran/graphman-copy-same branch from 668b7c4 to 042641c Compare May 31, 2024 14:49

zorancv marked this pull request as ready for review May 31, 2024 15:01

lutter requested changes Jun 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graphman copy indexing improvements #5425

Graphman copy indexing improvements #5425

zorancv commented May 17, 2024 •

edited

lutter Jun 6, 2024

lutter Jun 6, 2024

lutter Jun 6, 2024

lutter Jun 8, 2024

lutter Jun 8, 2024

lutter Jun 8, 2024

lutter Jun 8, 2024

lutter Jun 8, 2024

lutter Jun 8, 2024

lutter Jun 8, 2024

Graphman copy indexing improvements #5425

Are you sure you want to change the base?

Graphman copy indexing improvements #5425

Conversation

zorancv commented May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorancv commented May 17, 2024 •

edited