Copy subgraph data in batches when grafting #2293

lutter · 2021-03-19T23:46:36Z

These changes make it so that the data copy for grafting is done in batches to avoid long-running transactions. It is now also permissible to graft subgraphs across shards.

Note that this PR sits on top of #2268 since it relies on its changes

leoyvens

This foreign schema thing is some dark sorcery!

store/postgres/migrations/2021-03-12-070453_add_copy_state/up.sql

leoyvens · 2021-03-25T20:21:08Z

store/postgres/src/catalog.rs

+        .filter(nsp::nspname.eq(namespace.as_str()))
+        .count()
+        .get_result::<i64>(conn)?
+        > 0)


You can do diesel::dsl::exists(nsp::table.filter(...)).

Neat! Changed.

leoyvens · 2021-03-25T20:24:41Z

store/postgres/src/catalog.rs

+        .filter(ft::foreign_table_schema.eq(src.namespace.as_str()))
+        .count()
+        .get_result::<i64>(conn)?
+        > 0;


Changed too

leoyvens · 2021-03-25T20:31:29Z

store/postgres/src/copy.rs

+                .filter(cs::src.eq(self.src.site.id))
+                .filter(cs::finished_at.is_null())
+                .count()
+                .get_result::<i64>(conn)?;


Again exists.

leoyvens · 2021-03-25T21:17:09Z

store/postgres/src/relational_queries.rs

+        out.push_sql(" where vid >= ");
+        out.push_bind_param::<BigInt, _>(&self.first_vid)?;
+        out.push_sql(" and vid < ");
+        out.push_bind_param::<BigInt, _>(&self.last_vid)?;


last_vid isn't really the last vid but the one more than the last, I think this would be more readable if this where <= last_vid and last_vid was passed in as the actual last vid.

True; I changed that.

leoyvens · 2021-03-25T21:19:24Z

store/postgres/src/copy.rs

+        .load::<MaxVid>(conn)?
+        .first()
+        .map(|v| v.max_vid)
+        .unwrap_or(0);


If the table is empty, I believe the target_vid should be -1.

True; that saves an unnecessary attempt to copy vid = 0

leoyvens · 2021-03-25T21:20:17Z

store/postgres/src/copy.rs

+    }
+
+    fn finished(&self) -> bool {
+        self.next_vid >= self.target_vid


My understanding is that next_vid has not yet been copied, and we want to copy up to and including target_vid, so we're only finished after next_vid > target_vid.

Yes, I made changes to that logic yesterday - I went from current_vid to next_vid because I thought it would be clearer, but didn't catch all the places that affected. Changed

leoyvens · 2021-03-25T21:25:51Z

store/postgres/src/copy.rs

+            self.dst.as_ref(),
+            &self.src,
+            self.next_vid,
+            self.next_vid + self.batch_size,


We're not limiting by target_vid here, I think that'd be good to make the behavior more predictable.

Yes, that's actually a pretty serious bug since we would end up copying data we should not be copying.

leoyvens · 2021-03-25T21:28:56Z

store/postgres/src/deployment_store.rs

+                dst.revert_block(&conn, &dst.site.deployment, block_to_revert)?;
+                Layout::revert_metadata(&conn, &dst.site.deployment, block_to_revert)?;
+                info!(logger, "Rewound subgraph to block {}", block.number;
+                      "time_ms" => start.elapsed().as_millis());


It'd be good to see more justification for this step in the comment.

I've actually been meaning to get rid of this and simply forgot. It was left over from when we copied everything and then got rid of the excess because copying the complex metadata structure up to just some block was too cumbersome. I've removed that in favor of copying just what we need.

As it turns out, reverting the data is important since it needs to unclamp versions that are deleted in the future. In theory, CopyEntityBatchQuery could do that during copying, but the SQL to do that becomes pretty unwieldy. Maybe as an optimization in the future.

leoyvens · 2021-03-25T21:38:44Z

store/postgres/src/subgraph_store.rs

            }
            None => None,
        };
-        store.start_subgraph(logger, site, graft_base)
+        store.start_subgraph(logger, site.clone(), graft_base)?;
+        self.primary_conn()?.copy_finished(site.as_ref())


It reads a bit weird to execute this unconditionally even though most subgraphs won't be copied when deployed, maybe this copy_finished call can be moved into start_subgraph?

Unfortunately, this can't go into start_subgraph since it manipulates a different database.

lutter · 2021-03-26T00:53:07Z

Added more commits to address all the review comments.

leoyvens · 2021-03-26T17:55:30Z

store/postgres/src/deployment_store.rs

                let start = Instant::now();
                let block_to_revert: BlockNumber = (block.number + 1)
                    .try_into()
                    .expect("block numbers fit into an i32");
                dst.revert_block(&conn, &dst.site.deployment, block_to_revert)?;
-                Layout::revert_metadata(&conn, &dst.site.deployment, block_to_revert)?;


Note to self: Remember this interaction if we ever add support for removing data sources.

lutter · 2021-03-27T02:13:14Z

I made one very small change to this PR: moving active_copies to the public database schema, and adding a cancelled_at column so that ongoing copy processes can be signalled to stop copying. The actual use of that is in #2313

lutter · 2021-04-01T02:19:18Z

Fixed a mistake in the copy_table_state table where next_vid and target_vid were int instead of int8

lutter force-pushed the lutter/deployment2 branch from 4507eec to 3060635 Compare March 19, 2021 23:49

lutter force-pushed the lutter/graft branch from 4a111d0 to a118e80 Compare March 19, 2021 23:49

lutter force-pushed the lutter/deployment2 branch from 3060635 to ae4fb48 Compare March 24, 2021 00:50

lutter force-pushed the lutter/graft branch from a118e80 to 7d1ba4d Compare March 24, 2021 00:50

lutter force-pushed the lutter/deployment2 branch from ae4fb48 to 6bb91b5 Compare March 25, 2021 02:14

lutter force-pushed the lutter/graft branch from 7d1ba4d to 377e218 Compare March 25, 2021 02:14

leoyvens self-requested a review March 25, 2021 17:17

leoyvens requested changes Mar 25, 2021

View reviewed changes

lutter mentioned this pull request Mar 25, 2021

Copy subgraphs between shards #2313

Merged

leoyvens reviewed Mar 26, 2021

View reviewed changes

leoyvens approved these changes Mar 26, 2021

View reviewed changes

lutter force-pushed the lutter/deployment2 branch from 6bb91b5 to 42d1960 Compare March 26, 2021 22:20

lutter force-pushed the lutter/graft branch 3 times, most recently from 405ec3a to 69fd7f3 Compare March 27, 2021 02:12

lutter force-pushed the lutter/deployment2 branch from 42d1960 to ba77f20 Compare March 29, 2021 17:53

lutter force-pushed the lutter/graft branch from 69fd7f3 to 51d1b5b Compare March 29, 2021 17:53

lutter force-pushed the lutter/deployment2 branch from ba77f20 to 46b19f4 Compare March 30, 2021 23:58

lutter force-pushed the lutter/graft branch from 51d1b5b to 01b5233 Compare March 31, 2021 00:00

lutter changed the base branch from lutter/deployment2 to master March 31, 2021 00:00

lutter force-pushed the lutter/graft branch 2 times, most recently from 0533d5a to 4ff0f40 Compare April 1, 2021 02:15

lutter force-pushed the lutter/graft branch 3 times, most recently from 70062ae to d20df11 Compare April 7, 2021 22:40

lutter added 2 commits April 20, 2021 15:53

graph, store: Copy subgraph data in batches

ff19fcd

store: Use the copy module to copy subgraph data for grafting

5942ef6

lutter added 9 commits April 20, 2021 15:53

store: Do not remove deployments that are the source of an active copy

cf18a5d

store: Log progress while copying subgraph data

b68b4f8

store: Use Site.id in deployment::exists

a42e3e8

store: Make data copying possible across shards

530928e

store: Map the metadata from foreign databases into each shard

dee37e3

store: Copy metadata across shards and allow cross-shard grafting

0ff3163

store: Use 'exists' instead of 'count(*) > 0' in a few places

cb97f52

store: Make sure we only copy the data that we set out to copy

1ac003d

store: Copy only necessary DDS so we don't need to revert them

674efa6

lutter force-pushed the lutter/graft branch from d20df11 to 674efa6 Compare April 20, 2021 23:48

lutter merged commit 674efa6 into master Apr 21, 2021

lutter deleted the lutter/graft branch April 21, 2021 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy subgraph data in batches when grafting #2293

Copy subgraph data in batches when grafting #2293

lutter commented Mar 19, 2021 •

edited

Loading

leoyvens left a comment

leoyvens Mar 25, 2021

lutter Mar 25, 2021

leoyvens Mar 25, 2021

lutter Mar 25, 2021

leoyvens Mar 25, 2021

lutter Mar 25, 2021

leoyvens Mar 25, 2021

lutter Mar 25, 2021

leoyvens Mar 25, 2021

lutter Mar 25, 2021 •

edited

Loading

leoyvens Mar 25, 2021

lutter Mar 25, 2021

leoyvens Mar 25, 2021

lutter Mar 25, 2021

leoyvens Mar 25, 2021

lutter Mar 26, 2021

lutter Mar 26, 2021

leoyvens Mar 26, 2021

leoyvens Mar 25, 2021

lutter Mar 26, 2021

lutter commented Mar 26, 2021

leoyvens Mar 26, 2021

lutter commented Mar 27, 2021 •

edited

Loading

lutter commented Apr 1, 2021

Copy subgraph data in batches when grafting #2293

Copy subgraph data in batches when grafting #2293

Conversation

lutter commented Mar 19, 2021 • edited Loading

leoyvens left a 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

Choose a reason for hiding this comment

lutter Mar 25, 2021 • edited Loading

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

Choose a reason for hiding this comment

lutter commented Mar 26, 2021

Choose a reason for hiding this comment

lutter commented Mar 27, 2021 • edited Loading

lutter commented Apr 1, 2021

lutter commented Mar 19, 2021 •

edited

Loading

lutter Mar 25, 2021 •

edited

Loading

lutter commented Mar 27, 2021 •

edited

Loading