From 2559b84049f8a1311fd1e4c905313f3837c86557 Mon Sep 17 00:00:00 2001 From: Marco Slot Date: Sat, 28 Apr 2018 17:51:19 +0200 Subject: [PATCH] Drop shards as current user instead of super user --- src/backend/distributed/Makefile | 4 ++- .../distributed/citus--7.4-1--7.4-2.sql | 35 +++++++++++++++++++ src/backend/distributed/citus.control | 2 +- .../commands/drop_distributed_table.c | 15 +++++++- .../master/master_delete_protocol.c | 21 +++++------ .../multi_transactional_drop_shards.out | 30 ++++++++++++++++ .../multi_unsupported_worker_operations.out | 2 +- .../sql/multi_transactional_drop_shards.sql | 14 ++++++++ 8 files changed, 109 insertions(+), 14 deletions(-) create mode 100644 src/backend/distributed/citus--7.4-1--7.4-2.sql diff --git a/src/backend/distributed/Makefile b/src/backend/distributed/Makefile index 4adb22b30..44f692404 100644 --- a/src/backend/distributed/Makefile +++ b/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 diff --git a/src/backend/distributed/citus--7.4-1--7.4-2.sql b/src/backend/distributed/citus--7.4-1--7.4-2.sql new file mode 100644 index 000000000..7ba6169db --- /dev/null +++ b/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'; diff --git a/src/backend/distributed/citus.control b/src/backend/distributed/citus.control index 5493b8234..5926e7513 100644 --- a/src/backend/distributed/citus.control +++ b/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 diff --git a/src/backend/distributed/commands/drop_distributed_table.c b/src/backend/distributed/commands/drop_distributed_table.c index 734d478d3..6237e206f 100644 --- a/src/backend/distributed/commands/drop_distributed_table.c +++ b/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); diff --git a/src/backend/distributed/master/master_delete_protocol.c b/src/backend/distributed/master/master_delete_protocol.c index 6a801e6c1..9aa21625b 100644 --- a/src/backend/distributed/master/master_delete_protocol.c +++ b/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); diff --git a/src/test/regress/expected/multi_transactional_drop_shards.out b/src/test/regress/expected/multi_transactional_drop_shards.out index 233f3a811..7d5385382 100644 --- a/src/test/regress/expected/multi_transactional_drop_shards.out +++ b/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; diff --git a/src/test/regress/expected/multi_unsupported_worker_operations.out b/src/test/regress/expected/multi_unsupported_worker_operations.out index 0eaea9517..3c7c547a6 100644 --- a/src/test/regress/expected/multi_unsupported_worker_operations.out +++ b/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 ------- diff --git a/src/test/regress/sql/multi_transactional_drop_shards.sql b/src/test/regress/sql/multi_transactional_drop_shards.sql index 0cb32b972..839cc7b14 100644 --- a/src/test/regress/sql/multi_transactional_drop_shards.sql +++ b/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;