Thank you, James!

Would love to get your feedback, @bharatnc and @alexey-milovidov. Switching to a shared lock demonstrated a significant performance boost in our case.

Workflow [PR], commit [a9265bb]

Summary: ❌

Hey @Avogar

I noticed I made a somewhat nuanced conflict with your changes here from last week #83877

I've tried to resolve that in a way I think is valid but I'm open to your feedback on it. Basically I'm taking the columns_and_secondary_indices_sizes_mutex while holding a shared lock of the parts mutex (rather than a unique lock). I then remove the explicit unlock which you'd added.

I think we shouldn't need the unlock since now the parts lock is shared, SELECTs and other read only operations can still make progress while this lock is held, but this would mean INSERTs for example can't progress under that circumstance.

I did some experiments with it - https://pastila.nl/?010a5e28/09fb8843f199ea5f21e56ff25a3ea8f2#inJq/SZSB0djgkosoErDwg==

And yes it makes queries with lots of parts in the table ~50% faster

I also saw that INSERT became slower, 5-6x, Yes, the INSERT is artificial, it create parts with only one row, but still, maybe we call lockParts where we can call readLockParts instead?

@jawm it will be interesting to see your numbers as well

I did some experiments with it - https://pastila.nl/?010a5e28/09fb8843f199ea5f21e56ff25a3ea8f2#inJq/SZSB0djgkosoErDwg==
And yes it makes queries with lots of parts in the table ~50% faster

Thanks for those experiments. I did try to add a benchmark test locally but it seemed like I was going to have to modify the actual framework a bit so I dropped it, but that did show a healthy speedup. Also fwiw you're making 10k parts, these tables on our side have uh, quite a bit more than that (100k) 🙈

I also saw that INSERT became slower, 5-6x, Yes, the INSERT is artificial, it create parts with only one row, but still, maybe we call lockParts where we can call readLockParts instead?

@jawm it will be interesting to see your numbers as well

So we made these changes in two phases

1. Adding the shared mutex / shared locking of the parts list gave us a ~3x improvement to leaf select durations.
2. Making a shared copy of the parts list between all selects gave us a further ~40% reduction to leaf select durations

I realize those numbers are extremely vague, but basically it resolved a significant scaling issue we were having.

As for INSERT performance, we do typically insert batches of a million rows so whatever cost we incurred was considered acceptable. I'm looking at a chart of insert durations since January, when we started having this scaling issue, and I see that insert durations have actually barely changed (while we saw SELECT degrade a lot until we made those fixes).

As for lockParts vs readLockParts, I think we must call lockParts, since an INSERT is actually modifying the parts list (by adding new parts), so it would not be safe to do that concurrently with another insert or merge which would also be mutating the parts list at the same time.

IMO the extra cost of an INSERT is in making an extra copy of the parts vector, which actually you've already got happening on every single SELECT query currently. So it basically should just be moving that cost from one place to another. If you had a usecase where there's more inserts than selects then I guess it's a bad change, but for us at least we get a lot more selects. Possibly I am missing something else that adds cost too, lmk what you think

Thanks for the reviews :)

Thanks for sharing!

As for lockParts vs readLockParts, I think we must call lockParts, since an INSERT is actually modifying the parts list (by adding new parts), so it would not be safe to do that concurrently with another insert or merge which would also be mutating the parts list at the same time.

You are right, I was not suggesting to use readLockParts in INSERT, but more of a find missing places where we can use readLockParts instead, maybe there are some left.

IMO the extra cost of an INSERT is in making an extra copy of the parts vector, which actually you've already got happening on every single SELECT query currently. So it basically should just be moving that cost from one place to another. If you had a usecase where there's more inserts than selects then I guess it's a bad change, but for us at least we get a lot more selects. Possibly I am missing something else that adds cost too, lmk what you think

I doubt that copying vector will slows INSERTs 5x times, maybe there is something else?

You are right, I was not suggesting to use readLockParts in INSERT, but more of a find missing places where we can use readLockParts instead, maybe there are some left.

Ah right I see what you mean. For the most part the compiler already forced this; Because I removed const from lockParts, all our non-const functions already had to be changed to take a read lock instead. I've done a quick scan of remaining callsites and perhaps some of them could be changed, but my approach was mainly to keep the diff as small as possible to make it easier to validate and review. I was identifying things to change according to hotspots I was seeing on flamegraphs, and I don't see any remaining problems from remaining calls to lockParts.

I doubt that copying vector will slows INSERTs 5x times, maybe there is something else?

I'm looking now at a flamegraph (CPU) of insert queries from our production cluster and see ~8% of cycles going into this recomputeSharedPartsList. So I could imagine that in a benchmark where you do lots of small inserts to a table which has a lot of parts, that the percentage could easily be quite a lot higher than that, but I've not looked especially closely at that scenario. On that flamegraph I don't see any other significant hotspots on the codepaths which I've modified.
Considering that I previously could see selects spending ~40% of their overall duration just copying this vector, I don't think it's too crazy to think it could explain why we see a large part of insert duration for this particular benchmark doing the same operation.
If you want I can try to dig a bit deeper about what's happening there.

Thanks! Indeed, looks like copying of parts is the only added overhead, and it is enough to give 5.x slowdown, by improving MarkRanges copying we can improve it - #86933

I'm not really sure how to interpret these test failures

test_cancel_freeze seems to fail in the cleanup step to drop the shadow directory. Considering it only fails in one of the sanitizer builds but not of the others, I'm hoping maybe it's just something flaky?

As for the AST fuzzer, it fails on an assertion seemingly in tcp handler code. Nothing in the stack trace looks close to code that I've touched.

So:

• The INSERT performance restored (now shared parts updated on first SELECT, and each write lock resets it)
• Redundant copy of RangesInDataParts has been removed which gives 3.5x performance benefit with 100K parts for simple SELECT (when partition pruning removes everything except one part)

And it is ready to land

Final results - SELECT became 8x faster (was only 2x in the initial version) - https://pastila.nl/?003033a7/21217bba3c0e5174038bb4f998d6c930#HxLYLrj0MFz2a283lYl/Ug==

We should also remove shared_ptr<RangesInDataParts>, MergeTreeDataSelectExecutor::filterPartsByPartition should accept const DataPartsVector & not RangesInDataParts, but there are some interface issues that makes it not so simple

Woah, that's awesome, thanks for helping to get it over the line :)
I guess we can look forward to a further improvement then with your extra changes