From 9bba8bb4e897a055256453c349ce4b1ef7bb9947 Mon Sep 17 00:00:00 2001 From: Marco Slot Date: Tue, 29 Sep 2020 16:51:26 +0200 Subject: [PATCH] Remove master_drop_sequences --- .../distributed/operations/delete_protocol.c | 81 ++----------------- .../distributed/sql/citus--9.4-1--9.5-1.sql | 2 + .../sql/downgrades/citus--9.5-1--9.4-1.sql | 9 +++ .../sql/udfs/citus_drop_trigger/9.5-1.sql | 33 ++++++++ .../sql/udfs/citus_drop_trigger/latest.sql | 9 --- src/test/regress/expected/multi_extension.out | 3 +- .../regress/expected/multi_mx_metadata.out | 33 -------- src/test/regress/sql/multi_mx_metadata.sql | 14 ---- 8 files changed, 51 insertions(+), 133 deletions(-) create mode 100644 src/backend/distributed/sql/udfs/citus_drop_trigger/9.5-1.sql diff --git a/src/backend/distributed/operations/delete_protocol.c b/src/backend/distributed/operations/delete_protocol.c index 0cd762f9a..35ae17604 100644 --- a/src/backend/distributed/operations/delete_protocol.c +++ b/src/backend/distributed/operations/delete_protocol.c @@ -252,86 +252,15 @@ master_drop_all_shards(PG_FUNCTION_ARGS) /* - * master_drop_sequences attempts to drop a list of sequences on worker nodes. - * The "IF EXISTS" clause is used to permit dropping sequences even if they may not - * exist. If the commands fail on the workers, the operation is rolled back. - * If ddl propagation (citus.enable_ddl_propagation) is set to off, then the function - * returns without doing anything. + * master_drop_sequences was previously used to drop sequences on workers + * when using metadata syncing. + * + * It may still be called when dropping objects during CREATE EXTENSION, + * hence the function remains in place. */ Datum master_drop_sequences(PG_FUNCTION_ARGS) { - ArrayType *sequenceNamesArray = PG_GETARG_ARRAYTYPE_P(0); - Datum sequenceNameDatum = 0; - bool isNull = false; - StringInfo dropSeqCommand = makeStringInfo(); - - if (!CitusHasBeenLoaded()) - { - /* ignore calls during CREATE EXTENSION citus */ - PG_RETURN_VOID(); - } - - CheckCitusVersion(ERROR); - - /* - * Do nothing if DDL propagation is switched off or we're not on - * the coordinator. Here we prefer to not error out on the workers - * because this function is called on every dropped sequence and - * we don't want to mess up the sequences that are not associated - * with distributed tables. - */ - if (!EnableDDLPropagation || !IsCoordinator()) - { - PG_RETURN_VOID(); - } - - /* iterate over sequence names to build single command to DROP them all */ - ArrayIterator sequenceIterator = array_create_iterator(sequenceNamesArray, 0, NULL); - while (array_iterate(sequenceIterator, &sequenceNameDatum, &isNull)) - { - if (isNull) - { - ereport(ERROR, (errmsg("unexpected NULL sequence name"), - errcode(ERRCODE_INVALID_PARAMETER_VALUE))); - } - - text *sequenceNameText = DatumGetTextP(sequenceNameDatum); - Oid sequenceOid = ResolveRelationId(sequenceNameText, true); - if (OidIsValid(sequenceOid)) - { - /* - * This case (e.g., OID is valid) could only happen when a user manually calls - * the UDF. So, ensure that the user has right to drop the sequence. - * - * In case the UDF is called via the DROP trigger, the OID wouldn't be valid since - * the trigger is called after DROP happens. - */ - EnsureSequenceOwner(sequenceOid); - } - - /* append command portion if we haven't added any sequence names yet */ - if (dropSeqCommand->len == 0) - { - appendStringInfoString(dropSeqCommand, "DROP SEQUENCE IF EXISTS"); - } - else - { - /* otherwise, add a comma to separate subsequent sequence names */ - appendStringInfoChar(dropSeqCommand, ','); - } - - appendStringInfo(dropSeqCommand, " %s", TextDatumGetCString(sequenceNameText)); - } - - if (dropSeqCommand->len != 0) - { - appendStringInfoString(dropSeqCommand, " CASCADE"); - - SendCommandToWorkersWithMetadata(DISABLE_DDL_PROPAGATION); - SendCommandToWorkersWithMetadata(dropSeqCommand->data); - } - PG_RETURN_VOID(); } diff --git a/src/backend/distributed/sql/citus--9.4-1--9.5-1.sql b/src/backend/distributed/sql/citus--9.4-1--9.5-1.sql index bccfa5f68..a5356bb19 100644 --- a/src/backend/distributed/sql/citus--9.4-1--9.5-1.sql +++ b/src/backend/distributed/sql/citus--9.4-1--9.5-1.sql @@ -3,6 +3,7 @@ -- bump version to 9.5-1 #include "udfs/undistribute_table/9.5-1.sql" #include "udfs/create_citus_local_table/9.5-1.sql" +#include "udfs/citus_drop_trigger/9.5-1.sql" SET search_path = 'pg_catalog'; @@ -14,5 +15,6 @@ DROP FUNCTION worker_execute_sql_task(bigint, integer, text, bool); DROP TRIGGER dist_authinfo_task_tracker_cache_invalidate ON pg_catalog.pg_dist_authinfo; DROP TRIGGER dist_poolinfo_task_tracker_cache_invalidate ON pg_catalog.pg_dist_poolinfo; DROP FUNCTION task_tracker_conninfo_cache_invalidate(); +DROP FUNCTION master_drop_sequences(text[]); RESET search_path; diff --git a/src/backend/distributed/sql/downgrades/citus--9.5-1--9.4-1.sql b/src/backend/distributed/sql/downgrades/citus--9.5-1--9.4-1.sql index a2a9ef18f..f22225d35 100644 --- a/src/backend/distributed/sql/downgrades/citus--9.5-1--9.4-1.sql +++ b/src/backend/distributed/sql/downgrades/citus--9.5-1--9.4-1.sql @@ -2,6 +2,8 @@ SET search_path = 'pg_catalog'; +#include "../udfs/citus_drop_trigger/9.0-1.sql" + -- Check if user has any citus local tables. -- If not, DROP create_citus_local_table UDF and continue safely. -- Otherwise, raise an exception to stop the downgrade process. @@ -78,6 +80,13 @@ CREATE TRIGGER dist_authinfo_task_tracker_cache_invalidate ON pg_catalog.pg_dist_authinfo FOR EACH STATEMENT EXECUTE PROCEDURE task_tracker_conninfo_cache_invalidate(); +CREATE FUNCTION master_drop_sequences(sequence_names text[]) + RETURNS void + LANGUAGE C STRICT + AS 'MODULE_PATHNAME', $$master_drop_sequences$$; +COMMENT ON FUNCTION master_drop_sequences(text[]) + IS 'drop specified sequences from the cluster'; + RESET search_path; DROP FUNCTION pg_catalog.undistribute_table(table_name regclass); diff --git a/src/backend/distributed/sql/udfs/citus_drop_trigger/9.5-1.sql b/src/backend/distributed/sql/udfs/citus_drop_trigger/9.5-1.sql new file mode 100644 index 000000000..da9e3c683 --- /dev/null +++ b/src/backend/distributed/sql/udfs/citus_drop_trigger/9.5-1.sql @@ -0,0 +1,33 @@ +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 + FOR v_obj IN SELECT * FROM pg_event_trigger_dropped_objects() + WHERE object_type IN ('table', 'foreign table') + LOOP + -- first drop the table and metadata on the workers + -- then drop all the shards on the workers + -- finally remove the pg_dist_partition entry on the coordinator + PERFORM master_remove_distributed_table_metadata_from_workers(v_obj.objid, v_obj.schema_name, v_obj.object_name); + PERFORM master_drop_all_shards(v_obj.objid, v_obj.schema_name, v_obj.object_name); + PERFORM master_remove_partition_metadata(v_obj.objid, v_obj.schema_name, v_obj.object_name); + END LOOP; + + -- remove entries from citus.pg_dist_object for all dropped root (objsubid = 0) objects + FOR v_obj IN SELECT * FROM pg_event_trigger_dropped_objects() + LOOP + PERFORM master_unmark_object_distributed(v_obj.classid, v_obj.objid, v_obj.objsubid); + END LOOP; +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/sql/udfs/citus_drop_trigger/latest.sql b/src/backend/distributed/sql/udfs/citus_drop_trigger/latest.sql index db8b13c09..da9e3c683 100644 --- a/src/backend/distributed/sql/udfs/citus_drop_trigger/latest.sql +++ b/src/backend/distributed/sql/udfs/citus_drop_trigger/latest.sql @@ -9,11 +9,6 @@ DECLARE 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 @@ -25,10 +20,6 @@ BEGIN PERFORM master_remove_partition_metadata(v_obj.objid, v_obj.schema_name, v_obj.object_name); END LOOP; - IF cardinality(sequence_names) > 0 THEN - PERFORM master_drop_sequences(sequence_names); - END IF; - -- remove entries from citus.pg_dist_object for all dropped root (objsubid = 0) objects FOR v_obj IN SELECT * FROM pg_event_trigger_dropped_objects() LOOP diff --git a/src/test/regress/expected/multi_extension.out b/src/test/regress/expected/multi_extension.out index 37f3b858e..6c31f5790 100644 --- a/src/test/regress/expected/multi_extension.out +++ b/src/test/regress/expected/multi_extension.out @@ -441,6 +441,7 @@ ALTER EXTENSION citus UPDATE TO '9.5-1'; SELECT * FROM print_extension_changes(); previous_object | current_object --------------------------------------------------------------------- + function master_drop_sequences(text[]) | function task_tracker_assign_task(bigint,integer,text) | function task_tracker_cleanup_job(bigint) | function task_tracker_conninfo_cache_invalidate() | @@ -449,7 +450,7 @@ SELECT * FROM print_extension_changes(); function worker_merge_files_and_run_query(bigint,integer,text,text) | | function create_citus_local_table(regclass) | function undistribute_table(regclass) -(8 rows) +(9 rows) DROP TABLE prev_objects, extension_diff; -- show running version diff --git a/src/test/regress/expected/multi_mx_metadata.out b/src/test/regress/expected/multi_mx_metadata.out index 3198db3c4..7a02d2080 100644 --- a/src/test/regress/expected/multi_mx_metadata.out +++ b/src/test/regress/expected/multi_mx_metadata.out @@ -334,42 +334,9 @@ SELECT raise_failed_aclcheck($$ $$); ERROR: must be owner of the object CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE -SELECT raise_failed_aclcheck($$ - SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']); -$$); -ERROR: must be owner of the object -CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE -SELECT raise_failed_aclcheck($$ - SELECT master_drop_sequences(ARRAY['distributed_mx_table_some_val_seq']); -$$); -ERROR: must be owner of the object -CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE -SELECT master_drop_sequences(ARRAY['non_existing_schema.distributed_mx_table_some_val_seq']); - master_drop_sequences ---------------------------------------------------------------------- - -(1 row) - -SELECT master_drop_sequences(ARRAY['']); -ERROR: invalid name syntax -SELECT master_drop_sequences(ARRAY['public.']); -ERROR: invalid name syntax -SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq_not_existing']); - master_drop_sequences ---------------------------------------------------------------------- - -(1 row) - -- make sure that we can drop unrelated tables/sequences CREATE TABLE unrelated_table(key serial); DROP TABLE unrelated_table; --- doesn't error out but it has no effect, so no need to error out -SELECT master_drop_sequences(NULL); - master_drop_sequences ---------------------------------------------------------------------- - -(1 row) - \c - postgres - :master_port -- finally make sure that the sequence remains SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass; diff --git a/src/test/regress/sql/multi_mx_metadata.sql b/src/test/regress/sql/multi_mx_metadata.sql index bc17ed5ad..b112a2148 100644 --- a/src/test/regress/sql/multi_mx_metadata.sql +++ b/src/test/regress/sql/multi_mx_metadata.sql @@ -210,25 +210,11 @@ $$); SELECT raise_failed_aclcheck($$ SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); $$); -SELECT raise_failed_aclcheck($$ - SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']); -$$); -SELECT raise_failed_aclcheck($$ - SELECT master_drop_sequences(ARRAY['distributed_mx_table_some_val_seq']); -$$); - -SELECT master_drop_sequences(ARRAY['non_existing_schema.distributed_mx_table_some_val_seq']); -SELECT master_drop_sequences(ARRAY['']); -SELECT master_drop_sequences(ARRAY['public.']); -SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq_not_existing']); -- make sure that we can drop unrelated tables/sequences CREATE TABLE unrelated_table(key serial); DROP TABLE unrelated_table; --- doesn't error out but it has no effect, so no need to error out -SELECT master_drop_sequences(NULL); - \c - postgres - :master_port -- finally make sure that the sequence remains