Potentially gigantic cross-database ID pluck in Geo
Summary
https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/workers/geo/repository_sync_worker.rb#L27
current_node.projects
.where.not(id: Geo::ProjectRegistry.synced.pluck(:project_id))
.order(last_repository_updated_at: :desc)
.limit(db_retrieve_batch_size)
.pluck(:id)current_node is a GeoNode, which maps to a table in the main database, as does the projects relation, which boils down to Project.all.
Geo::ProjectRegistry is in the separate, read-write, per-geo-secondary database. So we have to pluck the IDs here. But what happens when a couple of million projects have been synced? What sort of query do we generate?
Steps to reproduce
Sync a few million projects, try to sync a few more.
What is the current bug behavior?
Postgres will probably fail with a "query too large" error of some sort
What is the expected correct behavior?
We need to be able to scale to several million rows in the project_registry table. I'm pretty sure this doesn't.
Possible fixes
Could we use where(last_repository_updated_at: nil) instead?
Activity
/cc @stanhu @dbalexandre
mentioned in issue #3259 (closed)
Postgres can handle very big queries, although I agree this might not always be desirable. For instance, I did this:
$ irb irb(main):001:0> open('query', 'w').puts("SELECT COUNT(*) FROM projects WHERE id IN (#{1.upto(5_000_000).to_a.join(',')});") => nil irb(main):002:0> exit $ du -h query 37M query $ gdk psql -d gitlabhq_development (in /Users/seanmcgivern/Code/gitlab-development-kit) psql (9.6.3) Type "help" for help. gitlabhq_development=# \i gitlab/query count ------- 20 (1 row) Time: 7408.511 mshttps://dba.stackexchange.com/questions/131399/is-there-a-maximum-length-constraint-for-a-postgres-query says we're good up to a gigabyte, not that we should try it.
removed Platform label
It seems our Geo production-like testbed is suffering from this:
ActiveRecord::StatementInvalid PG::QueryCanceled: ERROR: canceling statement due to statement timeout : SELECT "projects"."id" FROM "projects" WHERE ("projects"."id" NOT IN (3897991, 3898219, 3898340, 3898140, 3898298, 3898192, 3898131, 3897712, 3897985, 3897937, 3898227, 3898064, 3897982, 3898349, 3898205, 3898282, 3898067, 3898180, 3898065, 3897867, 3898123, 3898276, 3898310, 3897970, 3898213, 3898265, 3897921, 3897976, 3898289, 3898256, 3898381, 3898149, 3898374, 3898008, 3898021, 3897963, 3897841, 3897519, 3897057, 3897943, 3897685, 3897753, 3897738, 3898358, 3897929, 3897677, 3897789, 3897123, 3897241, 3897834, 3897831, 3898364, 3897956, 3897895, 3897399, 3897388, 3897703, 3897757, 3897698, 3897935, 3897926, 3898203, 3897750, 3897762, 3897771, 3897343, 3897440, 3897353, 3897435, 3897547, 3897542, 3897815, 3897575, 3897587, 3897554, 3897596, 3897533, 3897526, 3897603, 3897515, 3897952, 3897489, 3897494, 3897094, 3897630, 3896974, 3897946, 3897461, 3897228, 3897322, 3897348, 3897384, 3897657, 3897649, 3897377, 3897645, 3897912, 3897456, 3896947, 3896909, 3897153, 3897711, 3897866, 3898196, 3898343, 3898280, 3898261, 3898063, 3898139, 3898387, 3898185, 3898055, 3897574, 3898148, 3898308, 3898217, 3898270, 3898228, 3898350, 3898211, 3897684, 3898297, 3897990, 3898372, 3898290, 3898254, 3897814, 3897977, 3898017, 3898380, 3898129, 3898007, 3897965, 3898022, 3897737, 3897731, 3897933, 3897942, 3897706, 3897279, 3897520, 3897840, 3897886, 3897962, 3897833, 3897893, 3897239, 3897398, 3897391, 3897651, 3897352, 3897702, 3897928, 3898361, 3897697, 3897763, 3897922, 3897920, 3897678, 3897672, 3897775, 3897916, 3897546, 3897538, 3897532, 3897583, 3897589, 3897525, 3898028, 3897602, 3897493, 3897227, 3897184, 3896973, 3897484, 3897323, 3897784, 3897345, 3897379, 3897777, 3897656, 3897374, 3897431, 3897372, 3897439, 3897457, 3897788, 3897466, 3897151, 3897488, 3897191, 3896770, 3897627, 3897124, 3897095, 3897090, 3896824, 3896636, 3896644, 3896908, 3897010, 3897003, 3898191, 3898341, 3898183, 3898200, 3898305, 3897710, 3898220, 3897736, 3898138, 3897865, 3898127, 3898295, 3898260, 3898212, 3898267, 3898147, 3898061, 3898274, 3898385, 3897989, 3897986, 3898285, 3898057, 3898377, 3898226, 3898348, 3897932, 3897733, 3898006, 3897981, 3898367, 3897972, 3897858, 3897967, 3897572, 3897568, 3897518, 3897683, 3897766, 3897938, 3898355, 3897467, 3897240, 3897513, 3897839, 3897960, 3897887, 3898025, 3897829, 3897894, 3897906, 3896972, 3897396, 3897653, 3897913, 3897745, 3897924, 3897761, 3897919, 3897674, 3897772, 3897430, 3897780, 3897654, 3897350, 3897911, 3897948, 3898027, 3897545, 3897563, 3897813, 3897584, 3897557, 3897593, 3897598, 3897529, 3897601, 3897491, 3897278, 3897150, 3897194, 3897628, 3897320, 3897787, 3897462, 3897375, 3897487, 3897380, 3898204, 3897438, 3897646, 3897235, 3897229, 3897188, 3896634, 3897169, 3897093, 3897120, 3897117, 3897114, 3897088, 3898306, 3898264, 3898195, 3898272, 3897751, 3898346, 3898339, 3897709, 3898060, 3898218, 3897864, 3897863, 3898178, 3898294, 3898388, 3898224, 3898188, 3898135, 3898255, 3898056, 3898146, 3898373, 3898130, 3897988, 3898382, 3898209, 3898005, 3898287, 3898353, 3897681, 3897735, 3897984, 3897979, 3897974, 3898019, 3897968, 3897573, 3897768, 3897930, 3897149, 3897517, 3897655, 3897395, 3897349, 3897626, 3897883, 3898365, 3897837, 3897835, 3897888, 3897959, 3897891, 3898026, 3897705, 3897701, 3897730, 3898202, 3897925, 3897746, 3897696, 3897941, 3897760, 3897675, 3898362, 3897918, 3897773, 3897508, 3897592, 3897543, 3897582, 3897567, 3897812, 3897564, 3897550, 3897539, 3897536, 3897530, 3897600, 3897492, 3897237, 3897907, 3896767, 3897193, 3897458, 3897321, 3897382, 3897786, 3897650, 3897390, 3897432, 3897437, 3897463, 3897465, 3897233, 3895996, 3897468, 3897115, 3897092, 3897085, 3897055, 3897051, 3898190, 3898304, 3898273, 3897734, 3898225, 3898268, 3897862, 3898134, 3897987, 3898344, 3898386, 3898059, 3898177, 3898369, 3898143, 3898003, 3898296, 3898197, 3897997, 3898214, 3898375, 3898258, 3898286, 3898352, 3897680, 3897975, 3897971, 3897857, 3897838, 3897571, 3897765, 3897770, 3897275, 3897569, 389751...Edited by Toon ClaesI think what we're seeing here is the cost of the
IN()clause being multiplied per row in the projects database.Where we get that huge list of values from (pluck vs. fdw) is less of an issue than the cost of checking array membership for every row in the projects table.
PG can optimise this from a linear array lookup to a lookup in a set -
EXPLAIN ANALYZEwould tell us if this is already happening or not. Certainly on my local DB, it's already happening.https://www.xaprb.com/blog/2006/06/28/why-large-in-clauses-are-problematic/ suggests using a temporary table instead of IN.
Can we do this when the primary database is readonly?We can't create a temporary table on the readonly copy of the primary, though.Edited by Nick ThomasI've set up FDW locally (on my gdk with 9 projects), using these instructions. The query to search for projects not present in the registry:
EXPLAIN ANALYZE SELECT * FROM app.projects WHERE NOT EXISTS ( SELECT 1 FROM project_registry WHERE project_registry.project_id = app.projects.id);Resulting in:
QUERY PLAN ---------------------------------------------------------------------------------------------------------------------- Hash Anti Join (cost=101.18..115.84 rows=103 width=708) (actual time=0.757..0.759 rows=1 loops=1) Hash Cond: (projects.id = project_registry.project_id) -> Foreign Scan on projects (cost=100.00..113.33 rows=111 width=708) (actual time=0.731..0.733 rows=9 loops=1) -> Hash (cost=1.08..1.08 rows=8 width=4) (actual time=0.014..0.014 rows=8 loops=1) Buckets: 1024 Batches: 1 Memory Usage: 9kB -> Seq Scan on project_registry (cost=0.00..1.08 rows=8 width=4) (actual time=0.009..0.011 rows=8 loops=1) Planning time: 0.269 ms Execution time: 0.972 ms (8 rows)Looks good, but how feasible would it be to get it into %10.0 ?
I'm wondering if we can do something a bit like this as a stopgap:
-- on RO replica SELECT id FROM projects ORDER BY last_repository_updated_at -- on node DB SELECT external_project_id FROM (VALUES (1),(2),...) AS t(external_project_id) LEFT OUTER JOIN project_registry ON t.external_project_id = project_registry.project_id WHERE project_registry.id IS NULL LIMIT <batch-size>It's still awful, but hopefully it wouldn't suffer from statement timeouts. WDYT?
I can run the above query locally (only a few columns in project_registry though) with 5 million
VALUES:values = (1..5_000_000).map { |id| "(#{id})" }.join(",") ; nil sql = "SELECT external_project_id FROM (VALUES #{values}) AS t(external_project_id) LEFT OUTER JOIN project_registry ON t.external_project_id = project_registry.project_id WHERE project_registry.id IS NULL" ; nil t1 = Time.now puts Geo::ProjectRegistry.connection.execute(sql).to_a.size # 4999972 records puts (Time.now - t1).to_i # 23 secondsIn no sense do I like this. Do we have other options for %10.0? What's the query timeout we need to work to? 5 seconds?
Edited by Nick Thomas/cc @yorickpeterse
What's the query timeout we need to work to? 5 seconds?
Currently it's 60 seconds but there are plans to reduce this to 15 seconds. However, we try to limit the amount of SQL time per page to 200 ms.
Regarding the query, why are we not storing Geo tables in the same database as GitLab? We seem to rely on data from GitLab tables so doing so would make it trivial to get the data. FDWs are also an option, but I have no idea if/how such an approach would scale.
@yorickpeterse the Geo secondary needs to be able to keep track of which projects it has synced, and which it has not. The database it has access to is read-only, so it has a separate RW database into which the
project_registrytable is placed.Since we introduced
RepositoryCreatedEvent, I think this query is only used to pick up projects that existed before the secondary was added (i.e., backfill), and those where theRepositoryCreatedEventfails to be handled for some reason. So perhaps we can consider alternatives that would allow us to remove this query entirely?mentioned in issue #3382
So perhaps we can consider alternatives that would allow us to remove this query entirely?
The less code/queries/etc the better, but I can see us still having to do something like this query. FDWs are an option we can definitely investigate for that.
@jarv can I get you to try the query in https://gitlab.com/gitlab-org/gitlab-ee/issues/3269#note_39686826 on
sync.geo.gitlab.com? It's ruby code for putting into the Rails console.It would also be good to see the output of
Geo::ProjectRegistry.count- I think it's just a few thousand rows at present.added requires production access label
@nick.thomas Here is the output:
irb(main):001:0> load '/tmp/q.rb' 4996316 31 => true irb(main):002:0> Geo::ProjectRegistry.count => 3684Thanks! That's better than what we have at the moment, so I'll prepare an MR for %10.0 in the absence of any other ideas.
Since we'll have a
LIMIT 1000clause in there too, this should be fairly insensitive to the number ofGeo::ProjectRegistryrows.
@nick.thomas Yeah, I think we might want to have a strategy that does something like what you suggest by scanning limited numbers of rows at a given time: https://gitlab.com/gitlab-org/gitlab-ee/issues/3269#note_39685758
changed milestone to %10.0
assigned to @nick.thomas
@stanhu just to be clear, we'll still be doing a gigantic cross-database ID pluck after https://gitlab.com/gitlab-org/gitlab-ee/issues/3269#note_39685758 - indeed, we'll be plucking more IDs than now. The difference is in how we query row inclusion, and the motivation is to get something minimal into %10.0
Thinking on it some more, the
VALUESapproach could be used in the existing direction (rather than plucking all the project IDs into the node DB, continue to pluck the project registry project IDs into the secondary). That's a smaller change that should perform at least as "well" as the query above.Edited by Nick Thomasmentioned in issue omnibus-gitlab#2760
mentioned in merge request !2887 (merged)
closed via merge request !2887 (merged)
mentioned in commit 4506615d
mentioned in commit 03d7ef04
mentioned in issue #3487
mentioned in issue #3645 (closed)
mentioned in merge request !3075 (merged)
