Avoid corrupting global context with trash settings

[tavplubix]

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed a bug that might lead to overriding global server settings with SELECT settings of a View or Materialized View, if this view was dropped asynchronously and the server was restarted before finishing background cleanup

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

For dropped tables we call createTableFromAST here with getContext() which gets the context from here, and it's indeed global context

The bug was introduced in #78637

[clickhouse-gh]

Workflow [PR], commit [41a77aa]

Summary: ❌

job_name	test_name	status	info	comment
Unit tests (tsan)		failure
Stateless tests (amd_binary, ParallelReplicas, s3 storage, parallel)		failure
	03271_benchmark_metrics	FAIL
	Exception in test runner	FAIL
	Killed by signal (in clickhouse-server.log or clickhouse-server.err.log)	FAIL
	Fatal messages (in clickhouse-server.log or clickhouse-server.err.log)	FAIL
Finish Workflow		failure
	python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py	failure

[Algunenano]

It seems this change is not necessary, right? The change to createTableFromAST creating a copy should cover all cases.

Also the CI is not happy about keeping a copy of the global context for some reason:

clickhouse local -q "SELECT value FROM system.build_options WHERE name = \'CXX_FLAGS\'" --stacktrace
Code: 49. DB::Exception: Context has expired. (LOGICAL_ERROR), Stack trace (when copying this message, always include the lines below):

0. /mnt/ch/ClickHouse/contrib/llvm-project/libcxx/include/__exception/exception.h:113: Poco::Exception::Exception(String const&, int) @ 0x0000000020e3a9f2
1. /mnt/ch/ClickHouse/src/Common/Exception.cpp:128: DB::Exception::Exception(DB::Exception::MessageMasked&&, int, bool) @ 0x00000000104b0eec
2. DB::Exception::Exception(String&&, int, String, bool) @ 0x0000000009180f5c
3. DB::Exception::Exception(PreformattedMessage&&, int) @ 0x0000000009180acc
4. DB::Exception::Exception<>(int, FormatStringHelperImpl<>) @ 0x000000000918e07b
5. /mnt/ch/ClickHouse/src/Interpreters/Context_fwd.h:42: DB::WithContextImpl<std::shared_ptr<DB::Context>>::getContext() const @ 0x000000001568443d
6. /mnt/ch/ClickHouse/src/Interpreters/DatabaseCatalog.cpp:234: DB::DatabaseCatalog::initializeAndLoadTemporaryDatabase() @ 0x0000000016a9ae5d
7. /mnt/ch/ClickHouse/programs/local/LocalServer.cpp:923: DB::LocalServer::processConfig() @ 0x00000000107d65df
8. /mnt/ch/ClickHouse/programs/local/LocalServer.cpp:626: DB::LocalServer::main(std::vector<String, std::allocator<String>> const&) @ 0x00000000107d1337
9. /mnt/ch/ClickHouse/base/poco/Util/src/Application.cpp:315: Poco::Util::Application::run() @ 0x0000000020f2f4f4
10. /mnt/ch/ClickHouse/programs/local/LocalServer.cpp:1178: mainEntryClickHouseLocal(int, char**) @ 0x00000000107dff72
11. /mnt/ch/ClickHouse/programs/main.cpp:380: main @ 0x00000000091767bf
12. ../sysdeps/nptl/libc_start_call_main.h:58: __libc_start_call_main @ 0x0000000000027675
13. ../csu/libc-start.c:360: __j0f128_finite@GLIBC_2.26 @ 0x0000000000027729
14. _start @ 0x000000000917502e

[tavplubix]

I looked through usages of getContext(), and some look suspicious, but they shouldn't be problematic. It's not necessary, but having a copy of the global context would be less dangerous

[alexey-milovidov]

Question: Why do we need to clean up tables that were not finished dropping before the server restart?
In that case, leaving these tables as garbage is simpler, and it would be a much preferred option.
Otherwise we have a liability with this complex and error-prone code.

[tavplubix]

Because of UNDROP and database_atomic_delay_before_drop_table_sec mostly (we would have too much garbage without this cleanup)

I wouldn't say it's complex and error-prone. I would rather remove this if we could

[tavplubix]

Unit tests (tsan) - #87123
Stateless tests (amd_binary, ParallelReplicas, s3 storage, parallel) - #87653