Skip to content

Commit

Permalink
[#12719] YSQL: Remove tablegroup_oid reloption
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@frozenspider
frozenspider committed Jun 29, 2022
1 parent 9c801cd commit f363a09
Show file tree
Hide file tree
Showing 31 changed files with 153 additions and 383 deletions.
12 changes: 0 additions & 12 deletions src/postgres/src/backend/access/common/reloptions.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,17 +377,6 @@ static relopt_int intRelOpts[] =

static relopt_oid oidRelOpts[] =
{
{
{
"tablegroup_oid",
"Tablegroup oid for this relation.",
RELOPT_KIND_HEAP | RELOPT_KIND_INDEX,
AccessExclusiveLock
},
InvalidOid,
FirstNormalObjectId,
OID_MAX
},
{
{
"colocation_id",
Expand Down Expand Up @@ -1593,7 +1582,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, vacuum_cleanup_index_scale_factor)},
{"colocated", RELOPT_TYPE_BOOL,
offsetof(StdRdOptions, colocated)},
{"tablegroup_oid", RELOPT_TYPE_OID, offsetof(StdRdOptions, tablegroup_oid)},
{"colocation_id", RELOPT_TYPE_OID, offsetof(StdRdOptions, colocation_id)},
{"table_oid", RELOPT_TYPE_OID, offsetof(StdRdOptions, table_oid)},
{"row_type_oid", RELOPT_TYPE_OID, offsetof(StdRdOptions, row_type_oid)},
Expand Down
4 changes: 2 additions & 2 deletions src/postgres/src/backend/bootstrap/bootparse.y
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ Boot_CreateStmt:
boot_reldesc = heap_create($2,
PG_CATALOG_NAMESPACE,
shared_relation ? GLOBALTABLESPACE_OID : 0,
InvalidOid, /* tablegroup */
InvalidOid, /* reltablegroup */
$3,
InvalidOid,
tupdesc,
Expand All @@ -280,7 +280,7 @@ Boot_CreateStmt:
id = heap_create_with_catalog($2,
PG_CATALOG_NAMESPACE,
shared_relation ? GLOBALTABLESPACE_OID : 0,
InvalidOid, /* tablegroup */
InvalidOid, /* reltablegroup */
$3,
$7,
InvalidOid,
Expand Down
8 changes: 4 additions & 4 deletions src/postgres/src/backend/catalog/aclchk.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4430,7 +4430,7 @@ pg_tablegroup_aclmask(Oid grp_oid, Oid roleid,
Acl *acl;
Oid ownerId;

// First check that the pg_tablegroup catalog actually exists.
/* First check that the pg_tablegroup catalog actually exists. */
if (!YbTablegroupCatalogExists) {
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
Expand All @@ -4442,7 +4442,7 @@ pg_tablegroup_aclmask(Oid grp_oid, Oid roleid,
return mask;

/*
* Get the tablegroup's ACL from pg_tablegroup
* Get the tablegroup's ACL from pg_yb_tablegroup
*/
tuple = SearchSysCache1(YBTABLEGROUPOID, ObjectIdGetDatum(grp_oid));
if (!HeapTupleIsValid(tuple))
Expand Down Expand Up @@ -5203,7 +5203,7 @@ pg_tablegroup_ownercheck(Oid grp_oid, Oid roleid)
HeapTuple grptuple;
Oid grpowner;

// Ensure that the pg_tablegroup catalog actually exists.
/* Ensure that the pg_yb_tablegroup catalog actually exists. */
if (!YbTablegroupCatalogExists) {
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
Expand All @@ -5214,7 +5214,7 @@ pg_tablegroup_ownercheck(Oid grp_oid, Oid roleid)
if (superuser_arg(roleid) || IsYbDbAdminUser(GetUserId()))
return true;

/* Search syscache for pg_tablegroup */
/* Search syscache for the tablegroup */
grptuple = SearchSysCache1(YBTABLEGROUPOID, ObjectIdGetDatum(grp_oid));
if (!HeapTupleIsValid(grptuple))
ereport(ERROR,
Expand Down
2 changes: 1 addition & 1 deletion src/postgres/src/backend/commands/cluster.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, char relpersistence,
OIDNewHeap = heap_create_with_catalog(NewHeapName,
namespaceid,
NewTableSpace,
InvalidOid, /* tablegroup */
InvalidOid, /* reltablegroup */
InvalidOid,
InvalidOid,
InvalidOid,
Expand Down
28 changes: 8 additions & 20 deletions src/postgres/src/backend/commands/indexcmds.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -650,8 +650,12 @@ DefineIndex(Oid relationId,
}

/* Use tablegroup of the indexed table, if any. */
Oid tablegroupId = YbTablegroupCatalogExists ?
get_tablegroup_oid_by_table_oid(relationId) : InvalidOid;
Oid tablegroupId = InvalidOid;
if (YbTablegroupCatalogExists && IsYBRelation(rel))
{
YbLoadTablePropertiesIfNeeded(rel, false /* allow_missing */);
tablegroupId = rel->yb_table_properties->tablegroup_oid;
}

if (OidIsValid(tablegroupId) && stmt->split_options)
{
Expand Down Expand Up @@ -695,19 +699,6 @@ DefineIndex(Oid relationId,
get_tablegroup_name(tablegroupId));
}

/*
* Prepend to stmt->options to be parsed for reloptions if tablegroupId is valid.
* We set this here instead of in parse_utilcmd since we need to do the above
* preprocessing and RBAC checks first. This still happens before transformReloptions
* so this option is included in the reloptions text array.
*/
if (OidIsValid(tablegroupId))
{
stmt->options = lcons(makeDefElem("tablegroup_oid",
(Node *) makeInteger(tablegroupId), -1),
stmt->options);
}

/*
* Force shared indexes into the pg_global tablespace. This is a bit of a
* hack but seems simpler than marking them in the BKI commands. On the
Expand Down Expand Up @@ -1577,11 +1568,8 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
{
YbLoadTablePropertiesIfNeeded(rel, false /* allow_missing */);

is_colocated = rel->yb_table_properties->is_colocated;

if (YbTablegroupCatalogExists)
tablegroupId = get_tablegroup_oid_by_table_oid(relId);

is_colocated = rel->yb_table_properties->is_colocated;
tablegroupId = rel->yb_table_properties->tablegroup_oid;
use_yb_ordering = !IsSystemRelation(rel);
}
RelationClose(rel);
Expand Down
110 changes: 22 additions & 88 deletions src/postgres/src/backend/commands/tablecmds.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,6 @@ struct DropRelationCallbackState
#define child_dependency_type(child_is_partition) \
((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)

static Oid GetTablegroupOidFromCreateStmt(CreateStmt *stmt);
static void truncate_check_rel(Relation rel);
static List *MergeAttributes(List *schema, List *supers, char relpersistence,
bool is_partition, List **supconstr, int *supOidCount);
Expand Down Expand Up @@ -770,7 +769,25 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
}
}

Oid tablegroupId = GetTablegroupOidFromCreateStmt(stmt);
Oid tablegroupId = stmt->tablegroup
? get_tablegroup_oid(stmt->tablegroup->tablegroup_name, false)
: InvalidOid;

/*
* Check permissions for tablegroup. To create a table within a tablegroup, a user must
* either be a superuser, the owner of the tablegroup, or have create perms on it.
*/
if (OidIsValid(tablegroupId) && !pg_tablegroup_ownercheck(tablegroupId, GetUserId()))
{
AclResult aclresult;

aclresult = pg_tablegroup_aclcheck(tablegroupId, GetUserId(), ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, OBJECT_YBTABLEGROUP,
get_tablegroup_name(tablegroupId));
}



Oid colocation_id = YbGetColocationIdFromRelOptions(stmt->options);

Expand Down Expand Up @@ -1196,53 +1213,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
return address;
}

/*
* Select tablegroup to use. If not specified, InvalidOid.
* Will produce an error if the pg_tablegroup system table has not been created.
* Disallows creating a table within a tablegroup without the correct permissions.
*/
static Oid
GetTablegroupOidFromCreateStmt(CreateStmt *stmt)
{
if (!stmt->tablegroup)
return InvalidOid;

if (!stmt->tablegroup->has_tablegroup)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("Cannot use NO TABLEGROUP in CREATE TABLE statement.")));

Oid tablegroupId = get_tablegroup_oid(stmt->tablegroup->tablegroup_name, false);

if (!OidIsValid(tablegroupId))
return InvalidOid;

/*
* Check permissions for tablegroup. To create a table within a tablegroup, a user must
* either be a superuser, the owner of the tablegroup, or have create perms on it.
*/
if (!pg_tablegroup_ownercheck(tablegroupId, GetUserId()))
{
AclResult aclresult;

aclresult = pg_tablegroup_aclcheck(tablegroupId, GetUserId(), ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, OBJECT_YBTABLEGROUP,
get_tablegroup_name(tablegroupId));
}

/*
* Prepend to stmt->options to be parsed for reloptions if tablegroupId is valid. We need
* to do the preprocessing and RBAC checks first. This still happens before
* transformReloptions so this option is included in the reloptions text array.
*/
stmt->options = lcons(makeDefElem("tablegroup_oid",
(Node *) makeInteger(tablegroupId), -1),
stmt->options);

return tablegroupId;
}

/*
* Emit the right error or warning message for a "DROP" command issued on a
* non-existent relation
Expand Down Expand Up @@ -7690,39 +7660,6 @@ YBMoveRelDependencies(Relation old_rel, Relation new_rel,
}
}

/*
* Filter the DefElem list, removing all reloptions that should not be
* carried over when cloning the relation.
*/
static List *
YBExcludeNonCopyableOptions(List *options)
{
if (options)
{
ListCell *prev = NULL;
ListCell *next;

for (ListCell *curr = list_head(options); curr != NULL; curr = next)
{
next = lnext(curr);
DefElem *elem = lfirst_node(DefElem, curr);

/*
* Tablegroup is stored as a reloption but should not be copied
* as such, there's a separate mechanism for it.
*/
if (strcmp(elem->defname, "tablegroup_oid") == 0)
{
options = list_delete_cell(options, curr, prev);
break; /* since we only have one case so far, we can stop early. */
}
else
prev = curr;
}
}
return options;
}

/*
* Primary key is an inherent part of a DocDB table, we can't literally "add"
* a primary key to an existing table.
Expand Down Expand Up @@ -7753,6 +7690,8 @@ YBCloneRelationSetPrimaryKey(Relation* mutable_rel, IndexStmt* stmt)
MemoryContext oldcxt, per_tup_cxt;
ObjectAddress result = InvalidObjectAddress, address;

Assert(IsYBRelation(*mutable_rel));

old_relid = RelationGetRelid(*mutable_rel);

constr = RelationGetDescr(*mutable_rel)->constr;
Expand Down Expand Up @@ -7860,13 +7799,10 @@ YBCloneRelationSetPrimaryKey(Relation* mutable_rel, IndexStmt* stmt)
create_stmt->options = untransformRelOptions(datum);
ReleaseSysCache(tuple);

create_stmt->options = YBExcludeNonCopyableOptions(create_stmt->options);

const Oid tablegroup_oid = RelationGetTablegroupOid(*mutable_rel);
const Oid tablegroup_oid = (*mutable_rel)->yb_table_properties->tablegroup_oid;
if (OidIsValid(tablegroup_oid))
{
create_stmt->tablegroup = makeNode(OptTableGroup);
create_stmt->tablegroup->has_tablegroup = true;
create_stmt->tablegroup->tablegroup_name = get_tablegroup_name(tablegroup_oid);
Assert(create_stmt->tablegroup->tablegroup_name);
}
Expand Down Expand Up @@ -8186,8 +8122,6 @@ YBCloneRelationSetPrimaryKey(Relation* mutable_rel, IndexStmt* stmt)
RenameRelation(rename_stmt);
CommandCounterIncrement();

idx_stmt->options = YBExcludeNonCopyableOptions(idx_stmt->options);

/* Create a new index taking up the freed name. */
idx_stmt->idxname = pstrdup(idx_orig_name);
idx_addr = DefineIndex(new_relid,
Expand Down
Loading

0 comments on commit f363a09

Please sign in to comment.