Improve cursor-based batched background migration sub-batch queries
Cursor-based BBMs build queries of the form
select * from <table>
where (<start_cursor_cols>) >= (start_cursor)
and (<end_cursor_cols>) <= (end_cursor)
and (<start_cursor_cols>) > (keyset_prev_end_cursor);If the cursor is a single column, this isn't a problem - postgres collapses to the tightest bound. But if the cursor is multicolumn, postgres can't optimize and we re-read data from the index starting at the beginning of the batch.
Fix the query text so that it only has a single lower and upper bound.
The following discussion from !235210 (merged) should be addressed:
-
@stomlinson started a discussion: (+2 comments)
There's a bit of an unfortunate query pattern here, and it's shared with all cursor-based migrations, but I don't think we should fix it in this MR.
underlying_relationisselect * from <table> where (<start_cursor_cols>) >= (start_cursor) and (<end_cursor_cols>) <= (end_cursor);so when the keyset sub-batch iteration happens, it rebuilds the query for each batch as
select * from <table> where (<start_cursor_cols>) >= (start_cursor) and (<end_cursor_cols>) <= (end_cursor) and (<start_cursor_cols>) > (keyset_prev_end_cursor);This causes the same "failure to collapse row conditions" problem as we saw with the view - each sub-batch will scan rows from
merge_request_diff_commitsstarting at the beginning of the outer batch.The inefficiency is bounded by the batch size so it's not a huge deal, but there might be about a 2x performance improvement available here.
It's tricky to fix - we could do
Gitlab::Pagination::Keyset::Iterator.new(scope: underlying_relation_without_start.order(cursor_columns), cursor: cursor_columns.zip(start_cursor).to_h), but the keyset iterator expects the cursor to sit before the start of the batch, so it would buildwhere (<cursor_cols>) > (cursor)and we would skip 1 row each batch.So I think it should get handled in a follow-up.