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.

Sign up for GitHub

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

Jump to bottom

[colocation] Properly set schema is_backfilling #5007

Closed
jaki opened this issue Jul 8, 2020 · 1 comment
Closed

[colocation] Properly set schema is_backfilling #5007

jaki opened this issue Jul 8, 2020 · 1 comment
Assignees
@jaki
Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug

Comments

@jaki
Copy link
Contributor

jaki commented Jul 8, 2020 •
edited by daniel-yb
Loading

Tablet::MarkBackfillDone appears to do no handling of colocated tables because it uses metadata_->primary_table_info(). Change it to receive at least the table id info of the index table in question so that it can modify the appropriate schema, not the schema of the colocation parent table.

I did not look into SetIsBackfilling(true), but it appears to be in catalog_manager.cc, and I assume it is done to the proper user table there.

SetIsBackfilling(false) in Tablet::MarkBackfillDone is done on the wrong table, as explained above. To get this information, it should be passed from async_rpc_tasks.cc AsyncBackfillDone::SendRequest. We can set alter_table_id or schema from there.

I haven't noticed consequences, but, theoretically, this would cause colocated table data from never being compacted because backfilling is set to true indefinitely (at least until drop). cc @amitanandaiyer

I discovered this while working on #4986.
@jaki jaki added kind/bug This issue is a bug area/ysql Yugabyte SQL (YSQL) labels Jul 8, 2020
@jaki jaki assigned ndeodhar and jaki Jul 8, 2020
@jaki jaki mentioned this issue Oct 30, 2020
Closed
@jaki
Copy link
Contributor Author

jaki commented Feb 1, 2021

Clarification: SetIsBackfilling is now SetRetainDeleteMarkers (commit 9f1733f). @zhaoalex noticed this.

zhaoalex added a commit that referenced this issue Feb 20, 2021
@zhaoalex
[#5007] ysql: Properly set retain_delete_markers for colocated tables
ab96372 
Summary:
Properly sets `retain_delete_markers` to false upon backfill done for colocated tables. We
pass in the table_id of the index table so we can target the schema of said table instead of the
colocated parent table. Adds a new ChangeMetadataRequestPB field `backfill_done_table_id`
to accommodate this.

Test Plan: `PgIndexBackfillTest.[Colocated]RetainDeleteMarkers`

Reviewers: rahuldesirazu, jason

Reviewed By: jason

Subscribers: ybase, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D10638
polarweasel pushed a commit to lizayugabyte/yugabyte-db that referenced this issue Mar 9, 2021
@zhaoalex
[yugabyte#5007] ysql: Properly set retain_delete_markers for colocate…
40cce0f 
…d tables

Summary:
Properly sets `retain_delete_markers` to false upon backfill done for colocated tables. We
pass in the table_id of the index table so we can target the schema of said table instead of the
colocated parent table. Adds a new ChangeMetadataRequestPB field `backfill_done_table_id`
to accommodate this.

Test Plan: `PgIndexBackfillTest.[Colocated]RetainDeleteMarkers`

Reviewers: rahuldesirazu, jason

Reviewed By: jason

Subscribers: ybase, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D10638
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jaki jaki

Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@zhaoalex @ndeodhar @jaki