Skip to content

Commit

Permalink
[#23230] [DB Clone][YSQL]: Fix Cloning to a time before drop table in…
Browse files Browse the repository at this point in the history
… case of colocated databases

Summary:
Prior to this diff, clone was failing when we were cloning to a time before a drop table operation happened on the source database. The problem is demonstrated in the colocated case only and shouldn't be mixed with D35178.
The root cause of the problem is that ysql_dump is unable to set the tablegroup_id correctly in the sql script. More specifically in the following snippet (notice the 0 which is a wrong default value):

```
-- For YB colocation backup, must preserve implicit tablegroup pg_yb_tablegroup oid
SELECT pg_catalog.binary_upgrade_set_next_tablegroup_oid('0'::pg_catalog.oid);
```

This is because calling `yb_table_properties()` on the dropped table will not set the tablegroup_id in the response.
As part of executing `yb_table_properties()`, `GetTableSchema` is called which in turn doesn't set the tablegroup_id properly if the requested table is a dropped one. The problem is that the tablegroup manager will not have an entry of the dropped table inside its inmemory maps and thus the tablegroup manager will return a nullptr when asked to get the tablegroup of the dropped table using `FindByTable`.

The diff fixes the issue by returning the tablegroup_id using the **colocation parent table id** rather than asking the tablegroup manager for the tablegroup_id. The idea is that we can always infer the tablegroup_id from the parent table as all the colocated tables share the same tablegroup_id. We also guarantee that the colocated database corresponding to the returned `tablegroup_id`  should still be there as the table is guaranteed not to be in a deleted state (There is a previous check in `GetTableSchemInternal` for that).

Test Plan:
Switched the test CloneAfterDropTable to a parametrized test where the parameter tests both colocation and non-colocation cases.

./yb_build.sh --cxx-test integration-tests_minicluster-snapshot-test --gtest_filter Colocation/PgCloneTestParam.CloneAfterDropTable/1

Reviewers: asrivastava, zdrudi

Reviewed By: zdrudi

Subscribers: ybase, slingam

Differential Revision: https://phorge.dev.yugabyte.com/D36702
  • Loading branch information
yamen-haddad committed Jul 30, 2024
1 parent e948dee commit 80835a8
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 4 deletions.
2 changes: 1 addition & 1 deletion src/yb/integration-tests/minicluster-snapshot-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ TEST_F(PgCloneTest, YB_DISABLE_TEST_IN_SANITIZERS(AbortMessage)) {

// The test is disabled in Sanitizers as ysql_dump fails in ASAN builds due to memory leaks
// inherited from pg_dump.
TEST_F(PgCloneTest, YB_DISABLE_TEST_IN_SANITIZERS(CloneAfterDropTable)) {
TEST_P(PgCloneTestWithColocatedDBParam, YB_DISABLE_TEST_IN_SANITIZERS(CloneAfterDropTable)) {
// Clone to a time before a drop table and check that the table exists with correct data.
// 1. Create a table and load some data.
// 2. Mark time t.
Expand Down
7 changes: 4 additions & 3 deletions src/yb/master/catalog_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7586,9 +7586,10 @@ Status CatalogManager::GetTableSchemaInternal(const GetTableSchemaRequestPB* req
resp->set_colocated(table->colocated());

if (table->IsColocatedUserTable()) {
auto* tablegroup = tablegroup_manager_->FindByTable(table->id());
if (tablegroup) {
resp->set_tablegroup_id(tablegroup->id());
// Set the tablegroup_id for colocated user tables only after Colocation is GA.
if (IsTablegroupParentTableId(table->LockForRead()->pb.parent_table_id())) {
resp->set_tablegroup_id(
GetTablegroupIdFromParentTableId(table->LockForRead()->pb.parent_table_id()));
}
}

Expand Down

0 comments on commit 80835a8

Please sign in to comment.