Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Phase 1][colocation] Set colocation_id consistently in schemas #5017

Closed
jaki opened this issue Jul 8, 2020 · 2 comments
Closed

[Phase 1][colocation] Set colocation_id consistently in schemas #5017

jaki opened this issue Jul 8, 2020 · 2 comments
Assignees
Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue

Comments

@jaki
Copy link
Contributor

jaki commented Jul 8, 2020

Jira Link: [DB-376](https://yugabyte.atlassian.net/browse/DB-376)
pgtable_id (now colocation_id) for schemas are haphazardly set.

  • Create table request from postgres does not set it
  • CatalogManager::CreateTable does not set it
  • RaftGroupMetadata::AddTable does set it
  • CatalogManager::AlterTable does set it
  • CatalogManager::AddIndexInfoToTable does not set it
  • CatalogManager::MarkIndexInfoFromTableForDeletion does not set it
  • RaftGroupMetadata::SetSchema does set it (since commit d92b234)

Ideally, pgtable_id would be set from wherever it originates from: postgres create table and alter table requests to master. That way, we don't have to set it ad hoc on master and tserver. However, this may cause backwards compatibility issues.

Another option is to set it from master: CatalogManager::CreateTable and ((CatalogManager::AddIndexInfoToTable and CatalogManager::MarkIndexInfoFromTableForDeletion) or AsyncAlterTable::SendRequest). This enables us to get rid of the ad hoc setting at tserver.

@jaki jaki added kind/enhancement This is an enhancement of an existing feature area/ysql Yugabyte SQL (YSQL) labels Jul 8, 2020
@frozenspider frozenspider added this to To do in Colocation via automation Apr 20, 2022
@tverona1 tverona1 changed the title [colocation] Set pgtable_id consistently in schemas [Phase 1][colocation] Set pgtable_id consistently in schemas May 17, 2022
@yugabyte-ci yugabyte-ci added the priority/medium Medium priority issue label May 17, 2022
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug and removed kind/enhancement This is an enhancement of an existing feature labels Jun 9, 2022
@frozenspider frozenspider added this to Backlog in YSQL via automation Jun 13, 2022
@yugabyte-ci yugabyte-ci added kind/enhancement This is an enhancement of an existing feature and removed kind/bug This issue is a bug labels Jul 9, 2022
@yugabyte-ci yugabyte-ci changed the title [Phase 1][colocation] Set pgtable_id consistently in schemas [Phase 1][colocation] Set colocation_id consistently in schemas Sep 12, 2022
@siddharth2411
Copy link
Contributor

As part of this issue, we essentially want to make sure that colocation_id do not get modified. To do this, we want to put checks on colocation_id wherever it can possibly get changed. There might be some other methods than the ones mentioned above which may change it.

  • Verify all the PB in CatalogManager that store colocation_id and ensure none of the function that use these PBs are modifying the colocation_id.
  • Verify that colocation_id is being set (correctly) only once in RaftGroupMetadata.

@frozenspider
Copy link
Contributor

Current state of things:

  • CatalogManager::CreateTable does set colocation_id - which makes sense since it's an entry point for all table creation.
  • RaftGroupMetadata::AddTable does not set it, but it does verify colocation_id remains unchanged
  • CatalogManager::AlterTable does not set it - AlterTableRequestPB has no field for that
  • CatalogManager::AddIndexInfoToTable does not set it, for the same reasons. Moreover, it's called from CatalogManager::CreateTable
  • CatalogManager::MarkIndexInfoFromTableForDeletion does not set it
  • RaftGroupMetadata::SetSchema does not set it, but it does verify colocation_id remains unchanged

To summarize, colocation_id is only set in CatalogManager::CreateTable which is consistent. Closing this issue.

YSQL automation moved this from Backlog to Done Jan 17, 2023
Colocation automation moved this from To do to Done Jan 17, 2023
@frozenspider frozenspider self-assigned this Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue
Projects
Status: Done
YSQL
  
Done
Development

No branches or pull requests

5 participants