databendlabs / databend

𝗗𝗮𝘁𝗮, 𝗔𝗻𝗮𝗹𝘆𝘁𝗶𝗰𝘀 & 𝗔𝗜. Modern alternative to Snowflake. Cost-effective and simple for massive-scale analytics. https://databend.com
https://docs.databend.com
Other
7.71k stars 732 forks source link

Bug of undrop table #16511

Open drmingdrmer opened 2 days ago

drmingdrmer commented 2 days ago

When undrop a table, the table id is removed from TableIdList, but TableIdList is not persisted in meta-service, which will cause the next undrop fail, because the last item in TableIdList is still the previous undropped table id. And will cause the next drop table operation to push a duplicated table id into TableIdList.

https://github.com/datafuselabs/databend/blob/d44b6753df76b8136158ec1596932479b5845552/src/meta/api/src/schema_api_impl.rs#L4213

The undrop is introduced in:

dosubot[bot] commented 2 days ago

To fix the issue where the TableIdList is not persisted in the meta-service after undropping a table, you need to ensure that the TableIdList is correctly updated and persisted. Here is a code snippet that demonstrates how to update the TableIdList:

async fn handle_undrop_table(
    kv_api: &(impl kvapi::KVApi<Error = MetaError> + ?Sized),
    req: impl UndropTableStrategy + std::fmt::Debug,
) -> Result<(), KVAppError> {
    let tenant_dbname_tbname = req.table_name_ident();

    let mut trials = txn_backoff(None, func_name!());
    loop {
        trials.next().unwrap()?.await;

        // Get db by name to ensure presence
        let (db_id, seq_db_meta) = req.refresh_target_db_meta(kv_api).await?;

        // Get table by tenant,db_id, table_name to assert presence.
        let dbid_tbname = DBIdTableName {
            db_id,
            table_name: tenant_dbname_tbname.table_name.clone(),
        };

        let (dbid_tbname_seq, table_id) = get_u64_value(kv_api, &dbid_tbname).await?;
        if !req.force_replace() {
            // If table id already exists, return error.
            if dbid_tbname_seq > 0 || table_id > 0 {
                return Err(KVAppError::AppError(AppError::UndropTableAlreadyExists(
                    UndropTableAlreadyExists::new(&tenant_dbname_tbname.table_name),
                )));
            }
        }

        // get table id list from _fd_table_id_list/db_id/table_name
        let dbid_tbname_idlist = TableIdHistoryIdent {
            database_id: db_id,
            table_name: tenant_dbname_tbname.table_name.clone(),
        };
        let (tb_id_list_seq, tb_id_list_opt): (_, Option<TableIdList>) =
            get_pb_value(kv_api, &dbid_tbname_idlist).await?;

        let mut tb_id_list = if tb_id_list_seq == 0 {
            return Err(KVAppError::AppError(AppError::UndropTableHasNoHistory(
                UndropTableHasNoHistory::new(&tenant_dbname_tbname.table_name),
            )));
        } else {
            tb_id_list_opt.ok_or_else(|| {
                KVAppError::AppError(AppError::UndropTableHasNoHistory(
                    UndropTableHasNoHistory::new(&tenant_dbname_tbname.table_name),
                ))
            })?
        };

        // Update the table id list
        tb_id_list.append(table_id);

        // Persist the updated table id list
        let txn_req = TxnRequest {
            condition: vec![
                txn_cond_seq(&dbid_tbname_idlist, Eq, tb_id_list_seq),
            ],
            if_then: vec![
                txn_op_put(&dbid_tbname_idlist, serialize_struct(&tb_id_list)?),
            ],
            else_then: vec![],
        };

        let (succ, _responses) = send_txn(kv_api, txn_req).await?;
        if succ {
            return Ok(());
        }
    }
}

This code ensures that the TableIdList is updated and persisted correctly when a table is undropped, preventing failures in subsequent undrop operations and avoiding duplicate table IDs during drop operations [1][2].

To continue talking to Dosu, mention @dosu.

dantengsky commented 1 day ago

Did I miss something? The version that the PR (https://github.com/datafuselabs/databend/pull/15186) is based on also did not persist the TableIdList (i.e., the kv pair _fd_table_id_list/db_id/table_name -> tb_id_list).

https://github.com/datafuselabs/databend/blob/330581025813a4425ab3b81b3de36890e5cb6330/src/meta/api/src/schema_api_impl.rs#L1853-L1872

drmingdrmer commented 1 day ago

Did I miss something? The version that the PR (#15186) is based on also did not persist the TableIdList (i.e., the kv pair _fd_table_id_list/db_id/table_name -> tb_id_list).

https://github.com/datafuselabs/databend/blob/330581025813a4425ab3b81b3de36890e5cb6330/src/meta/api/src/schema_api_impl.rs#L1853-L1872

That's probably a bug that already exists before #15186