From 1b5560b2f7711ef6b0b57101c8ad079920964e79 Mon Sep 17 00:00:00 2001 From: Burak Yucesoy Date: Mon, 15 May 2017 02:52:39 +0300 Subject: [PATCH 1/3] Fix OwnerName function to work with schemas We incorrectly try to use relation cache to find particular schema's owner and when we cannot find the schema in the relation cache(i.e always), we automatically used current user as the schema's owner. This means we always created schemas in the data nodes with current user. With this patch we started to use namespace cache to find schemas. --- src/backend/distributed/metadata/metadata_sync.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/backend/distributed/metadata/metadata_sync.c b/src/backend/distributed/metadata/metadata_sync.c index b59bdcd37..822f5b4e6 100644 --- a/src/backend/distributed/metadata/metadata_sync.c +++ b/src/backend/distributed/metadata/metadata_sync.c @@ -53,7 +53,7 @@ static List * SequenceDDLCommandsForTable(Oid relationId); static void EnsureSupportedSequenceColumnType(Oid sequenceOid); static Oid TypeOfColumn(Oid tableId, int16 columnId); static char * TruncateTriggerCreateCommand(Oid relationId); -static char * OwnerName(Oid objectId); +static char * SchemaOwnerName(Oid objectId); static bool HasMetadataWorkers(void); @@ -893,7 +893,7 @@ CreateSchemaDDLCommand(Oid schemaId) } schemaNameDef = makeStringInfo(); - ownerName = OwnerName(schemaId); + ownerName = SchemaOwnerName(schemaId); appendStringInfo(schemaNameDef, CREATE_SCHEMA_COMMAND, schemaName, ownerName); return schemaNameDef->data; @@ -968,19 +968,19 @@ TruncateTriggerCreateCommand(Oid relationId) /* - * OwnerName returns the name of the owner of the specified object. + * SchemaOwnerName returns the name of the owner of the specified schema. */ static char * -OwnerName(Oid objectId) +SchemaOwnerName(Oid objectId) { HeapTuple tuple = NULL; Oid ownerId = InvalidOid; char *ownerName = NULL; - tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(objectId)); + tuple = SearchSysCache1(NAMESPACEOID, ObjectIdGetDatum(objectId)); if (HeapTupleIsValid(tuple)) { - ownerId = ((Form_pg_class) GETSTRUCT(tuple))->relowner; + ownerId = ((Form_pg_namespace) GETSTRUCT(tuple))->nspowner; } else { @@ -989,6 +989,8 @@ OwnerName(Oid objectId) ownerName = GetUserNameFromId(ownerId, false); + ReleaseSysCache(tuple); + return ownerName; } From 5a3a32d6df179fefe8ffb2a2e521fa0f913d7e8b Mon Sep 17 00:00:00 2001 From: Burak Yucesoy Date: Mon, 15 May 2017 03:05:01 +0300 Subject: [PATCH 2/3] Quote schema's owner name When we propogate the schema creation command to data nodes we add schema's owner name too. Before this patch, we did not quote the owner's name which causes problems with the names containing characters like '-'. --- src/backend/distributed/metadata/metadata_sync.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/distributed/metadata/metadata_sync.c b/src/backend/distributed/metadata/metadata_sync.c index 822f5b4e6..af1aa5257 100644 --- a/src/backend/distributed/metadata/metadata_sync.c +++ b/src/backend/distributed/metadata/metadata_sync.c @@ -885,7 +885,7 @@ CreateSchemaDDLCommand(Oid schemaId) { char *schemaName = get_namespace_name(schemaId); StringInfo schemaNameDef = NULL; - char *ownerName = NULL; + const char *ownerName = NULL; if (strncmp(schemaName, "public", NAMEDATALEN) == 0) { @@ -893,7 +893,7 @@ CreateSchemaDDLCommand(Oid schemaId) } schemaNameDef = makeStringInfo(); - ownerName = SchemaOwnerName(schemaId); + ownerName = quote_identifier(SchemaOwnerName(schemaId)); appendStringInfo(schemaNameDef, CREATE_SCHEMA_COMMAND, schemaName, ownerName); return schemaNameDef->data; From 577ffb2bf278d20feaa97eec5173b0f6b61d3301 Mon Sep 17 00:00:00 2001 From: Burak Yucesoy Date: Mon, 15 May 2017 11:16:59 +0300 Subject: [PATCH 3/3] Add tests for non-default schema owner --- .../regress/expected/multi_schema_support.out | 59 +++++++++++++++++++ src/test/regress/sql/multi_schema_support.sql | 28 +++++++++ 2 files changed, 87 insertions(+) diff --git a/src/test/regress/expected/multi_schema_support.out b/src/test/regress/expected/multi_schema_support.out index 1208f8159..44dd95066 100644 --- a/src/test/regress/expected/multi_schema_support.out +++ b/src/test/regress/expected/multi_schema_support.out @@ -1084,3 +1084,62 @@ SET search_path TO public; ALTER TABLE test_schema_support.nation_hash SET SCHEMA public; WARNING: not propagating ALTER ... SET SCHEMA commands to worker nodes HINT: Connect to worker nodes directly to manually change schemas of affected objects. +-- we will use this function in next test +CREATE FUNCTION run_command_on_coordinator_and_workers(p_sql text) +RETURNS void LANGUAGE plpgsql AS $$ +BEGIN + EXECUTE p_sql; + PERFORM run_command_on_workers(p_sql); +END;$$; +-- test schema propagation with user other than current user +SELECT run_command_on_coordinator_and_workers('CREATE USER "test-user"'); +NOTICE: not propagating CREATE ROLE/USER commands to worker nodes +HINT: Connect to worker nodes directly to manually create all necessary users and roles. +CONTEXT: SQL statement "CREATE USER "test-user"" +PL/pgSQL function run_command_on_coordinator_and_workers(text) line 3 at EXECUTE + run_command_on_coordinator_and_workers +---------------------------------------- + +(1 row) + +SELECT run_command_on_coordinator_and_workers('GRANT ALL ON DATABASE postgres to "test-user"'); + run_command_on_coordinator_and_workers +---------------------------------------- + +(1 row) + +CREATE SCHEMA schema_with_user AUTHORIZATION "test-user"; +CREATE TABLE schema_with_user.test_table(column1 int); +SELECT create_reference_table('schema_with_user.test_table'); + create_reference_table +------------------------ + +(1 row) + +-- verify that owner of the created schema is test-user +\c - - - :worker_1_port +\dn schema_with_user + List of schemas + Name | Owner +------------------+----------- + schema_with_user | test-user +(1 row) + +\c - - - :master_port +-- we do not use run_command_on_coordinator_and_workers here because when there is CASCADE, it causes deadlock +DROP OWNED BY "test-user" CASCADE; +NOTICE: drop cascades to table schema_with_user.test_table +SELECT run_command_on_workers('DROP OWNED BY "test-user" CASCADE'); + run_command_on_workers +---------------------------------- + (localhost,57637,t,"DROP OWNED") + (localhost,57638,t,"DROP OWNED") +(2 rows) + +SELECT run_command_on_coordinator_and_workers('DROP USER "test-user"'); + run_command_on_coordinator_and_workers +---------------------------------------- + +(1 row) + +DROP FUNCTION run_command_on_coordinator_and_workers(p_sql text); diff --git a/src/test/regress/sql/multi_schema_support.sql b/src/test/regress/sql/multi_schema_support.sql index 2215eead5..a5864a562 100644 --- a/src/test/regress/sql/multi_schema_support.sql +++ b/src/test/regress/sql/multi_schema_support.sql @@ -737,3 +737,31 @@ SET citus.task_executor_type TO "real-time"; -- we expect that it will warn out SET search_path TO public; ALTER TABLE test_schema_support.nation_hash SET SCHEMA public; + +-- we will use this function in next test +CREATE FUNCTION run_command_on_coordinator_and_workers(p_sql text) +RETURNS void LANGUAGE plpgsql AS $$ +BEGIN + EXECUTE p_sql; + PERFORM run_command_on_workers(p_sql); +END;$$; + +-- test schema propagation with user other than current user +SELECT run_command_on_coordinator_and_workers('CREATE USER "test-user"'); +SELECT run_command_on_coordinator_and_workers('GRANT ALL ON DATABASE postgres to "test-user"'); + +CREATE SCHEMA schema_with_user AUTHORIZATION "test-user"; +CREATE TABLE schema_with_user.test_table(column1 int); +SELECT create_reference_table('schema_with_user.test_table'); + +-- verify that owner of the created schema is test-user +\c - - - :worker_1_port +\dn schema_with_user + +\c - - - :master_port +-- we do not use run_command_on_coordinator_and_workers here because when there is CASCADE, it causes deadlock +DROP OWNED BY "test-user" CASCADE; +SELECT run_command_on_workers('DROP OWNED BY "test-user" CASCADE'); +SELECT run_command_on_coordinator_and_workers('DROP USER "test-user"'); + +DROP FUNCTION run_command_on_coordinator_and_workers(p_sql text);