This is an automated comment for commit 7900fe5 with description of existing statuses. It's updated for the latest CI running

⏳ Click here to open a full report in a separate page

To apply it back please introduce a test which will show performance gains.

@alesapin which one of the failures had been related to this PR? (also, even though it is not related, please take a look at #61973 (comment))

To apply it back please introduce a test which will show performance gains.

fsync is a tricky thing
And likely this test will be came flaky, and eventually will be removed due to flakiness of perf tests.

Also can you please tag me on reverts, since I saw this due to pure luck.

which one of the failures had been related to this PR?

It broke our private repo sync, the PR was merged without checking "A sync" check which was not green, the reviewer should fix the sync before the PR is merged.

It broke our private repo sync, the PR was merged without checking "A sync" check which was not green, the reviewer should fix the sync before the PR is merged.

@kssenii thanks! Any insights on what had been broken? Maybe some functions signatures overlaps?

( Or maybe even this one - 6c3db34 :) )

There was a small conflict in MergeTreeData.cpp, near these lines in your PR https://github.com/ClickHouse/ClickHouse/pull/61973/files#diff-b7d33a7abda5f3365d5e4fb58ca98b756f6401372b2e2d5eb449a2bd3444fb5eL6636-L6638, but it was non-obvious that quick conflict resolution will not break anything, so to avoid private sync being stuck - it was better to revert.

@azat This change:

1. Makes code worse and more complex
2. Nobody ever reported similar issue
3. No proofs that it helps even for the case that you are describing.

Makes code worse and more complex

I have some doubts about this as well (#61973 (comment)), but I ran out of ideas, the code does not allow to make this clean without rewriting (usually I'm trying to keep patches small, this avoids extra conflicts)

If you have any ideas, please let me know.

Nobody ever reported similar issue

Sure, I doubt that there are lots of users who enables fsync on production

No proofs that it helps even for the case that you are describing.

It is actually pretty straightforward - if you are holding lock during rename you depends on how long will the rename takes, and under load it could take awhile, especially on HDDs (it also depends on the journal settings for ext4, and where this journal is located and so on and so forth). And it will be magnitude worser in case of fsync_part_directory=1.

And things could be even worse by some "tricky" HDDs, for instance I found out that on one of our production we had T10 protection enabled, which slows down disks a lot (up to 2x)

Sure it is nice to have a performance test, but it is unlikely will be deterministic, and eventually will be removed (I already had such experience in the past with complex tests). I will try to come up with some numbers for v2.