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

[Phase 1][YSQL][Colocation] Get rid of tablegroup_oid reloption #12719

Closed
frozenspider opened this issue May 31, 2022 · 0 comments
Closed

[Phase 1][YSQL][Colocation] Get rid of tablegroup_oid reloption #12719

frozenspider opened this issue May 31, 2022 · 0 comments
Assignees
@frozenspider
Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue
Projects
YSQL Colocation

Comments

@frozenspider
Copy link
Contributor

frozenspider commented May 31, 2022 •
edited

Jira Link: DB-607

Description

Right now, tablegroup_oid is the only YB-speicifc option we store in reloptions rather than in YB-specific struct. There's little reason for that.
@frozenspider frozenspider added kind/enhancement This is an enhancement of an existing feature area/ysql Yugabyte SQL (YSQL) labels May 31, 2022
@frozenspider frozenspider added this to Backlog in YSQL via automation May 31, 2022
@frozenspider frozenspider added this to To do in Colocation via automation May 31, 2022
@frozenspider frozenspider self-assigned this May 31, 2022
@yugabyte-ci yugabyte-ci added the priority/medium Medium priority issue label May 31, 2022
@frozenspider frozenspider mentioned this issue May 31, 2022
Open
@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 moved this from To do to In progress in Colocation Jun 14, 2022
frozenspider added a commit that referenced this issue Jun 17, 2022
@frozenspider
[#12719] YSQL: Rework the way yb_table_properties are stored
5e23584 
Summary:
Previously, `YBCPgTableProperties` were fetched on demand from the tserver and transparently cached as a part of `PgApiImpl::GetTableDesc`. This has been reworked to be a lazily loaded part of `Relation` struct, and is thus now cached explicitly.
Note that cache invalidation (e.g. because of `catalog_version` change, or `CommandCounterIncrement`) naturally discards this field which is a desired behaviour most of the time.

I've added a TODO note of how should we optimize `Relation` creation, but since fetching `yb_table_properties` is hitting `PgTableDesc` cache most of the time, this is low-pri.

Since YB table properties are now accessible to PG directly, `YbIsUserTableColocated` was removed.

I decided to extract this into a separate diff while working on #12719 as I think this is a self-sufficient chunk of work that feels like a strict improvement over the way things were done before.

Test Plan: Existing tests. Also, manually tested that all places where this is used still work as before.

Reviewers: jason, dmitry, tverona, mihnea, myang

Reviewed By: myang

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D17692
@frozenspider frozenspider added kind/enhancement This is an enhancement of an existing feature and removed kind/bug This issue is a bug labels Jun 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 21, 2022
frozenspider added a commit that referenced this issue Jun 29, 2022
@frozenspider
[#12719] YSQL: Remove tablegroup_oid reloption
f363a09 
Summary:
At the time prior to this diff, `tablegroup_oid` remained the only YB-specifc property that we store in `reloptions` rather than in YB-specific struct.
Since master is always aware of the tablegroup, moved this to be exclusive to `YbTableProperties` and to be fetched from master.

One side effect of this change is that table's primary key is no longer shown as belonging to the tablegroup (since PK is a dummy thing in YB). Discussed this to be okay (and we can tweak this later if needed).

Test Plan: Existing tests

Reviewers: myang, tverona, mihnea, dmitry, jason

Reviewed By: jason

Subscribers: yql, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D17779
@yugabyte-ci yugabyte-ci changed the title [Phase 1][YSQL][Colocation] Get rid of tablegroup_oid reloption [Phase 1][YSQL][Colocation] Get rid of tablegroup_oid reloption Jun 29, 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 Jun 29, 2022
@yugabyte-ci yugabyte-ci changed the title [Phase 1][YSQL][Colocation] Get rid of tablegroup_oid reloption [Phase 1][YSQL][Colocation] Get rid of tablegroup_oid reloption Jun 29, 2022
frozenspider added a commit that referenced this issue Jul 1, 2022
@frozenspider
[#12719] YSQL: Tablegroup related cleanup
50f2372 
Summary:
This is a follow-up to 5e23584 / D17692 and f363a09 / D17779 that does some minor refactoring/cleanup discussed for those diffs.
Two style changes are:

* Removed `OptTableGroup` grammar node, replaced with a plain `char *` tablegroup name.
* Replaced `YbLoadTablePropertiesIfNeeded` with a more traditional lazy loading getter.

There are no functional changes.

Test Plan: Existing tests

Reviewers: jason, dmitry

Reviewed By: dmitry

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D17997
YSQL automation moved this from Backlog to Done Jul 1, 2022
Colocation automation moved this from In progress to Done Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@frozenspider frozenspider

Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature priority/medium Medium priority issue
Projects
Colocation
Done
YSQL
  
Done
Milestone
No milestone
Development

No branches or pull requests
2 participants
@frozenspider @yugabyte-ci