Merge pull request #2138 from citusdata/drop_shards_users

Make DROP TABLE connect as current user instead of superuser

src/backend/distributed/Makefile

@@ -15,7 +15,7 @@ EXTVERSIONS = 5.0 5.0-1 5.0-2  \
 	7.1-1 7.1-2 7.1-3 7.1-4 \
     7.2-1 7.2-2 7.2-3 \
 	7.3-1 7.3-2 7.3-3 \
-	7.4-1
+	7.4-1 7.4-2
 
 # All citus--*.sql files in the source directory
 DATA = $(patsubst $(citus_abs_srcdir)/%.sql,%.sql,$(wildcard $(citus_abs_srcdir)/$(EXTENSION)--*--*.sql))
@@ -195,6 +195,8 @@ $(EXTENSION)--7.3-3.sql: $(EXTENSION)--7.3-2.sql $(EXTENSION)--7.3-2--7.3-3.sql
 	cat $^ > $@
 $(EXTENSION)--7.4-1.sql: $(EXTENSION)--7.3-3.sql $(EXTENSION)--7.3-3--7.4-1.sql
 	cat $^ > $@
+$(EXTENSION)--7.4-2.sql: $(EXTENSION)--7.4-1.sql $(EXTENSION)--7.4-1--7.4-2.sql
+	cat $^ > $@
 
 NO_PGXS = 1
 

src/backend/distributed/citus--7.4-1--7.4-2.sql

@@ -0,0 +1,35 @@
+/* citus--7.4-1--7.4-2 */
+
+CREATE OR REPLACE FUNCTION pg_catalog.citus_drop_trigger()
+    RETURNS event_trigger
+    LANGUAGE plpgsql
+    SET search_path = pg_catalog
+    AS $cdbdt$
+DECLARE
+    v_obj record;
+    sequence_names text[] := '{}';
+    table_colocation_id integer;
+    propagate_drop boolean := false;
+BEGIN
+    -- collect set of dropped sequences to drop on workers later
+    SELECT array_agg(object_identity) INTO sequence_names
+    FROM pg_event_trigger_dropped_objects()
+    WHERE object_type = 'sequence';
+
+    FOR v_obj IN SELECT * FROM pg_event_trigger_dropped_objects()
+                 WHERE object_type IN ('table', 'foreign table')
+    LOOP
+        -- drop all shards and the metadata
+        PERFORM master_drop_all_shards(v_obj.objid, v_obj.schema_name, v_obj.object_name);
+        PERFORM master_drop_distributed_table_metadata(v_obj.objid, v_obj.schema_name, v_obj.object_name);
+    END LOOP;
+
+    IF cardinality(sequence_names) = 0 THEN
+        RETURN;
+    END IF;
+
+    PERFORM master_drop_sequences(sequence_names);
+END;
+$cdbdt$;
+COMMENT ON FUNCTION pg_catalog.citus_drop_trigger()
+    IS 'perform checks and actions at the end of DROP actions';

src/backend/distributed/citus.control

@@ -1,6 +1,6 @@
 # Citus extension
 comment = 'Citus distributed database'
-default_version = '7.4-1'
+default_version = '7.4-2'
 module_pathname = '$libdir/citus'
 relocatable = false
 schema = pg_catalog

src/backend/distributed/commands/drop_distributed_table.c

@@ -14,6 +14,7 @@
 #include "distributed/master_metadata_utility.h"
 #include "distributed/master_protocol.h"
 #include "distributed/metadata_sync.h"
+#include "distributed/multi_utility.h"
 #include "distributed/worker_transaction.h"
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
@@ -38,9 +39,21 @@ master_drop_distributed_table_metadata(PG_FUNCTION_ARGS)
 	char *schemaName = text_to_cstring(schemaNameText);
 	char *tableName = text_to_cstring(tableNameText);
 
-	EnsureCoordinator();
 	CheckCitusVersion(ERROR);
 
+	/*
+	 * The SQL_DROP trigger calls this function even for tables that are
+	 * not distributed. In that case, silently ignore. This is not very
+	 * user-friendly, but this function is really only meant to be called
+	 * from the trigger.
+	 */
+	if (!IsDistributedTable(relationId) || !EnableDDLPropagation)
+	{
+		PG_RETURN_VOID();
+	}
+
+	EnsureCoordinator();
+
 	CheckTableSchemaNameForDrop(relationId, &schemaName, &tableName);
 
 	DeletePartitionRow(relationId);

src/backend/distributed/master/master_delete_protocol.c

@@ -223,9 +223,18 @@ master_drop_all_shards(PG_FUNCTION_ARGS)
 	char *schemaName = text_to_cstring(schemaNameText);
 	char *relationName = text_to_cstring(relationNameText);
 
-	EnsureCoordinator();
 	CheckCitusVersion(ERROR);
 
+	/*
+	 * The SQL_DROP trigger calls this function even for tables that are
+	 * not distributed. In that case, silently ignore and return -1.
+	 */
+	if (!IsDistributedTable(relationId) || !EnableDDLPropagation)
+	{
+		PG_RETURN_INT32(-1);
+	}
+
+	EnsureCoordinator();
 	CheckTableSchemaNameForDrop(relationId, &schemaName, &relationName);
 
 	/*
@@ -325,12 +334,6 @@ CheckTableSchemaNameForDrop(Oid relationId, char **schemaName, char **tableName)
 
 		EnsureTableOwner(relationId);
 	}
-	else if (!superuser())
-	{
-		/* table does not exist, must be called from drop trigger */
-		ereport(ERROR, (errmsg("cannot drop distributed table metadata as a "
-							   "non-superuser")));
-	}
 }
 
 
@@ -382,7 +385,6 @@ DropShards(Oid relationId, char *schemaName, char *relationName,
 			StringInfo workerDropQuery = makeStringInfo();
 			MultiConnection *connection = NULL;
 			uint32 connectionFlags = FOR_DDL;
-			char *extensionOwner = CitusExtensionOwnerName();
 
 			char storageType = shardInterval->storageType;
 			if (storageType == SHARD_STORAGE_TABLE)
@@ -397,8 +399,7 @@ DropShards(Oid relationId, char *schemaName, char *relationName,
 								 quotedShardName);
 			}
 
-			connection = GetPlacementConnection(connectionFlags, shardPlacement,
-												extensionOwner);
+			connection = GetPlacementConnection(connectionFlags, shardPlacement, NULL);
 
 			RemoteTransactionBeginIfNecessary(connection);
 

src/test/regress/expected/multi_transactional_drop_shards.out

@@ -684,3 +684,33 @@ SELECT stop_metadata_sync_to_node('localhost', :worker_1_port);
  
 (1 row)
 
+-- test DROP TABLE as a non-superuser in a transaction block
+CREATE USER try_drop_table WITH LOGIN;
+NOTICE:  not propagating CREATE ROLE/USER commands to worker nodes
+HINT:  Connect to worker nodes directly to manually create all necessary users and roles.
+GRANT ALL ON SCHEMA public TO try_drop_table;
+SELECT run_command_on_workers('CREATE USER try_drop_table WITH LOGIN');
+      run_command_on_workers       
+-----------------------------------
+ (localhost,57637,t,"CREATE ROLE")
+ (localhost,57638,t,"CREATE ROLE")
+(2 rows)
+
+SELECT run_command_on_workers('GRANT ALL ON SCHEMA public TO try_drop_table');
+  run_command_on_workers   
+---------------------------
+ (localhost,57637,t,GRANT)
+ (localhost,57638,t,GRANT)
+(2 rows)
+
+\c - try_drop_table - :master_port
+BEGIN;
+CREATE TABLE temp_dist_table (x int, y int);
+SELECT create_distributed_table('temp_dist_table','x');
+ create_distributed_table 
+--------------------------
+ 
+(1 row)
+
+DROP TABLE temp_dist_table;
+END;

src/test/regress/expected/multi_unsupported_worker_operations.out

@@ -333,7 +333,7 @@ DROP TABLE mx_table;
 ERROR:  operation is not allowed on this node
 HINT:  Connect to the coordinator and run it again.
 CONTEXT:  SQL statement "SELECT master_drop_all_shards(v_obj.objid, v_obj.schema_name, v_obj.object_name)"
-PL/pgSQL function citus_drop_trigger() line 21 at PERFORM
+PL/pgSQL function citus_drop_trigger() line 17 at PERFORM
 SELECT count(*) FROM mx_table;
  count 
 -------

src/test/regress/sql/multi_transactional_drop_shards.sql

@@ -374,3 +374,17 @@ SELECT master_remove_node('localhost', :master_port);
 -- clean the workspace
 DROP TABLE transactional_drop_shards, transactional_drop_reference;
 SELECT stop_metadata_sync_to_node('localhost', :worker_1_port);
+
+-- test DROP TABLE as a non-superuser in a transaction block
+CREATE USER try_drop_table WITH LOGIN;
+GRANT ALL ON SCHEMA public TO try_drop_table;
+SELECT run_command_on_workers('CREATE USER try_drop_table WITH LOGIN');
+SELECT run_command_on_workers('GRANT ALL ON SCHEMA public TO try_drop_table');
+
+\c - try_drop_table - :master_port
+
+BEGIN;
+CREATE TABLE temp_dist_table (x int, y int);
+SELECT create_distributed_table('temp_dist_table','x');
+DROP TABLE temp_dist_table;
+END;