Merge pull request #61348 from rschu1ze/flaky-test_uncrop_query

Un-flake `test_undrop_query`

docs/en/operations/server-configuration-parameters/settings.md

@@ -933,9 +933,9 @@ Hard limit is configured via system tools
 
 ## database_atomic_delay_before_drop_table_sec {#database_atomic_delay_before_drop_table_sec}
 
-Sets the delay before remove table data in seconds. If the query has `SYNC` modifier, this setting is ignored.
+The delay before a table data is dropped in seconds. If the `DROP TABLE` query has a `SYNC` modifier, this setting is ignored.
 
-Default value: `480` (8 minute).
+Default value: `480` (8 minutes).
 
 ## database_catalog_unused_dir_hide_timeout_sec {#database_catalog_unused_dir_hide_timeout_sec}
 

docs/en/sql-reference/statements/undrop.md

@@ -13,13 +13,6 @@ a system table called `system.dropped_tables`.
 
 If you have a materialized view without a `TO` clause associated with the dropped table, then you will also have to UNDROP the inner table of that view.
 
-:::note
-UNDROP TABLE is experimental.  To use it add this setting: 
-```sql
-set allow_experimental_undrop_table_query = 1;
-```
-:::
-
 :::tip
 Also see [DROP TABLE](/docs/en/sql-reference/statements/drop.md)
 :::
@@ -32,60 +25,53 @@ UNDROP TABLE [db.]name [UUID '<uuid>'] [ON CLUSTER cluster]
 
 **Example**
 
-``` sql
-set allow_experimental_undrop_table_query = 1;
-```
-
 ```sql
-CREATE TABLE undropMe
+CREATE TABLE tab
 (
     `id` UInt8
 )
 ENGINE = MergeTree
-ORDER BY id
+ORDER BY id;
-```
+
+DROP TABLE tab;
 
-```sql
-DROP TABLE undropMe
-```
-```sql
 SELECT *
 FROM system.dropped_tables
-FORMAT Vertical
+FORMAT Vertical;
 ```
+
 ```response
 Row 1:
 ──────
 index:                 0
 database:              default
-table:                 undropMe
+table:                 tab
 uuid:                  aa696a1a-1d70-4e60-a841-4c80827706cc
 engine:                MergeTree
-metadata_dropped_path: /var/lib/clickhouse/metadata_dropped/default.undropMe.aa696a1a-1d70-4e60-a841-4c80827706cc.sql
+metadata_dropped_path: /var/lib/clickhouse/metadata_dropped/default.tab.aa696a1a-1d70-4e60-a841-4c80827706cc.sql
 table_dropped_time:    2023-04-05 14:12:12
 
 1 row in set. Elapsed: 0.001 sec. 
 ```
+
 ```sql
-UNDROP TABLE undropMe
+UNDROP TABLE tab;
-```
+
-```response
-Ok.
-```
-```sql
 SELECT *
 FROM system.dropped_tables
-FORMAT Vertical
+FORMAT Vertical;
-```
+
 ```response
 Ok.
 
 0 rows in set. Elapsed: 0.001 sec. 
 ```
+
 ```sql
-DESCRIBE TABLE undropMe
+DESCRIBE TABLE tab
-FORMAT Vertical
+FORMAT Vertical;
 ```
+
 ```response
 Row 1:
 ──────

src/Interpreters/DatabaseCatalog.cpp

@@ -1143,7 +1143,7 @@ void DatabaseCatalog::dequeueDroppedTableCleanup(StorageID table_id)
     TableMarkedAsDropped dropped_table;
     {
         std::lock_guard lock(tables_marked_dropped_mutex);
-        time_t latest_drop_time = std::numeric_limits<time_t>::min();
+        auto latest_drop_time = std::numeric_limits<time_t>::min();
         auto it_dropped_table = tables_marked_dropped.end();
         for (auto it = tables_marked_dropped.begin(); it != tables_marked_dropped.end(); ++it)
         {
@@ -1168,7 +1168,7 @@ void DatabaseCatalog::dequeueDroppedTableCleanup(StorageID table_id)
         }
         if (it_dropped_table == tables_marked_dropped.end())
             throw Exception(ErrorCodes::UNKNOWN_TABLE,
-                "The drop task of table {} is in progress, has been dropped or the database engine doesn't support it",
+                "Table {} is being dropped, has been dropped, or the database engine does not support UNDROP",
                 table_id.getNameForLogs());
         latest_metadata_dropped_path = it_dropped_table->metadata_path;
         String table_metadata_path = getPathForMetadata(it_dropped_table->table_id);

src/Interpreters/InterpreterUndropQuery.cpp

@@ -17,14 +17,16 @@ namespace ErrorCodes
     extern const int SUPPORT_IS_DISABLED;
 }
 
-InterpreterUndropQuery::InterpreterUndropQuery(const ASTPtr & query_ptr_, ContextMutablePtr context_) : WithMutableContext(context_), query_ptr(query_ptr_)
+InterpreterUndropQuery::InterpreterUndropQuery(const ASTPtr & query_ptr_, ContextMutablePtr context_)
+    : WithMutableContext(context_)
+    , query_ptr(query_ptr_)
 {
 }
 
-
 BlockIO InterpreterUndropQuery::execute()
 {
     getContext()->checkAccess(AccessType::UNDROP_TABLE);
+
     auto & undrop = query_ptr->as<ASTUndropQuery &>();
     if (!undrop.cluster.empty() && !maybeRemoveOnCluster(query_ptr, getContext()))
     {

tests/integration/test_undrop_query/test.py

@@ -29,30 +29,39 @@ def test_undrop_drop_and_undrop_loop(started_cluster):
         logging.info(
             "random_sec: " + random_sec.__str__() + ", table_uuid: " + table_uuid
         )
+
         node.query(
-            "create table test_undrop_loop"
+            "CREATE TABLE test_undrop_loop"
             + count.__str__()
             + " UUID '"
             + table_uuid
-            + "' (id Int32) Engine=MergeTree() order by id;"
+            + "' (id Int32) ENGINE = MergeTree() ORDER BY id;"
         )
-        node.query("drop table test_undrop_loop" + count.__str__() + ";")
+
+        node.query("DROP TABLE test_undrop_loop" + count.__str__() + ";")
+
         time.sleep(random_sec)
+
         if random_sec >= 5:
             error = node.query_and_get_error(
-                "undrop table test_undrop_loop"
+                "UNDROP TABLE test_undrop_loop"
                 + count.__str__()
-                + " uuid '"
+                + " UUID '"
                 + table_uuid
                 + "';"
             )
             assert "UNKNOWN_TABLE" in error
-        else:
+        elif random_sec <= 3:
+            # (*)
             node.query(
-                "undrop table test_undrop_loop"
+                "UNDROP TABLE test_undrop_loop"
                 + count.__str__()
-                + " uuid '"
+                + " UUID '"
                 + table_uuid
                 + "';"
             )
             count = count + 1
+        else:
+            pass
+            # ignore random_sec = 4 to account for communication delay with the database.
+            # if we don't do that, then the second case (*) may find the table already dropped and receive an unexpected exception from the database (Bug #55167)

tests/queries/0_stateless/02681_undrop_query.sql

@@ -85,5 +85,5 @@ drop table 02681_undrop_multiple;
 select table from system.dropped_tables where table = '02681_undrop_multiple' limit 1;
 undrop table 02681_undrop_multiple;
 select * from 02681_undrop_multiple order by id;
-undrop table 02681_undrop_multiple; -- { serverError 57 }
+undrop table 02681_undrop_multiple; -- { serverError TABLE_ALREADY_EXISTS }
 drop table 02681_undrop_multiple sync;