01193_metadata_loading is flaky (suppressed)

[Algunenano]

SELECT
  check_name,
  test_name,
  toStartOfDay(check_start_time) as t,
  count() as runs,
  100 * (countIf(test_status != 'OK' AND test_status != 'SKIPPED') AS f) / runs as failure_percentage
FROM checks
WHERE
    test_name LIKE '01193_metadata_loading%'
    AND pull_request_number = 0
    AND check_start_time > today() - interval 30 day
GROUP BY check_name, test_name, t
HAVING f > 0
ORDER by check_name, test_name, t

№	check_name	test_name	t	runs	failure_percentage
1	Stateless tests (aarch64)	01193_metadata_loading	2023-06-16 00:00:00	20	20
2	Stateless tests (aarch64)	01193_metadata_loading	2023-06-17 00:00:00	12	8.333333333333334
3	Stateless tests (aarch64)	01193_metadata_loading	2023-06-19 00:00:00	8	50
4	Stateless tests (release)	01193_metadata_loading	2023-06-12 00:00:00	13	7.6923076923076925
5	Stateless tests (release)	01193_metadata_loading	2023-06-17 00:00:00	14	7.142857142857143
6	Stateless tests (release, DatabaseOrdinary)	01193_metadata_loading	2023-06-17 00:00:00	14	14.285714285714286
7	Stateless tests (release, DatabaseOrdinary)	01193_metadata_loading	2023-06-19 00:00:00	8	25
8	Stateless tests (release, DatabaseReplicated) [2/4]	01193_metadata_loading	2023-06-16 00:00:00	20	10
9	Stateless tests (release, DatabaseReplicated) [2/4]	01193_metadata_loading	2023-06-19 00:00:00	8	12.5
10	Stateless tests (release, analyzer)	01193_metadata_loading	2023-06-17 00:00:00	14	7.142857142857143

[tavplubix]

The issue is temporarily suppressed by #51297. Tables loading slowdown still has to be investigated

[nickitat]

this code slows down detach several times for MT in this test. while the solution looks elegant it abuses exceptions. apparently it could be implemented as a thread local flag that we set-unset around calls that will lead to destructor calls? so we will basically get the same effect without paying cost of stack unwinding.
@CheSema wdyt?
update: we discussed it briefly with Sema. changing the interface seems to be the right approach, but it also seem to require a significant refactoring. I don't really want to do it just to fix the test. and make a crutch with thread local variable doesn't look reasonable now.

[CheSema]

It could be even more simple -- change the interface. Here we need more that buffer interface, we need add finalizeFile and abortFile calls. This is time consuming task with low popularity.
It is supposed to not to run very often. As I understand this is syntetic test with high rate detach.

[nickitat]

this function also gives some pretty noticeable overhead. every access to a global config's field is done within a critical section. probably we're better to return some read-only snapshot of global config from getConfig instead of AbstractConfiguration.

[nickitat]

this test continues to be flaky and there is no good way to fix it currently. at least that I'm aware of. and the test itself is pretty questionable. any objections to remove it?

@tavplubix @alexey-milovidov

[alexey-milovidov]

No objections, remove it.

[alexey-milovidov]

PS. The test was created to check if the loading of tables didn't accidentally become sequential instead of parallel, which was the case at least once.