From b12e77ab0e2588cd6e3c0c1453de66168030a823 Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Mon, 27 Nov 2017 11:43:23 -0700 Subject: [PATCH 1/3] Ensure unsupported VACUUMs don't go to workers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apparently these two blocks have been incorrect for nearly a year… --- src/backend/distributed/executor/multi_utility.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/backend/distributed/executor/multi_utility.c b/src/backend/distributed/executor/multi_utility.c index 0275c3d03..339e6e073 100644 --- a/src/backend/distributed/executor/multi_utility.c +++ b/src/backend/distributed/executor/multi_utility.c @@ -1557,6 +1557,8 @@ IsSupportedDistributedVacuumStmt(Oid relationId, VacuumStmt *vacuumStmt) errhint("Set citus.enable_ddl_propagation to true in order to " "send targeted %s commands to worker nodes.", stmtName))); + + return false; } if (vacuumStmt->options & VACOPT_VERBOSE) @@ -1564,6 +1566,8 @@ IsSupportedDistributedVacuumStmt(Oid relationId, VacuumStmt *vacuumStmt) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("the VERBOSE option is currently unsupported in " "distributed %s commands", stmtName))); + + return false; } return true; From 0eacf6bd9576921a608d9a828e67c7b5f5142c6e Mon Sep 17 00:00:00 2001 From: Jason Petersen Date: Mon, 27 Nov 2017 11:49:46 -0700 Subject: [PATCH 2/3] Refactor VacuumStmt checker to be single-return Decided this would be safer for the future (defaults to unsupported). --- .../distributed/executor/multi_utility.c | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/backend/distributed/executor/multi_utility.c b/src/backend/distributed/executor/multi_utility.c index 339e6e073..8863fcd37 100644 --- a/src/backend/distributed/executor/multi_utility.c +++ b/src/backend/distributed/executor/multi_utility.c @@ -1534,43 +1534,39 @@ static bool IsSupportedDistributedVacuumStmt(Oid relationId, VacuumStmt *vacuumStmt) { const char *stmtName = (vacuumStmt->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE"; + bool distributeStmt = false; if (vacuumStmt->relation == NULL) { - /* WARN and exit early for unqualified VACUUM commands */ + /* WARN for unqualified VACUUM commands */ ereport(WARNING, (errmsg("not propagating %s command to worker nodes", stmtName), errhint("Provide a specific table in order to %s " "distributed tables.", stmtName))); - - return false; } - - if (!OidIsValid(relationId) || !IsDistributedTable(relationId)) + else if (!OidIsValid(relationId) || !IsDistributedTable(relationId)) { - return false; + /* Nothing to do here; relation no longer exists or is not distributed */ } - - if (!EnableDDLPropagation) + else if (!EnableDDLPropagation) { - /* WARN and exit early if DDL propagation is not enabled */ + /* WARN if DDL propagation is not enabled */ ereport(WARNING, (errmsg("not propagating %s command to worker nodes", stmtName), errhint("Set citus.enable_ddl_propagation to true in order to " "send targeted %s commands to worker nodes.", stmtName))); - - return false; } - - if (vacuumStmt->options & VACOPT_VERBOSE) + else if (vacuumStmt->options & VACOPT_VERBOSE) { ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("the VERBOSE option is currently unsupported in " "distributed %s commands", stmtName))); - - return false; + } + else + { + distributeStmt = true; } - return true; + return distributeStmt; } From 6041f85b704ed7905d5ae3065e8dcbeeaa7a3835 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?mehmet=20furkan=20=C5=9Fahin?= Date: Tue, 28 Nov 2017 17:09:38 +0300 Subject: [PATCH 3/3] Add tests for non-propagated VACUUM/ANALYZE --- src/test/regress/expected/multi_utilities.out | 86 +++++++++++++++++++ src/test/regress/sql/multi_utilities.sql | 51 +++++++++++ 2 files changed, 137 insertions(+) diff --git a/src/test/regress/expected/multi_utilities.out b/src/test/regress/expected/multi_utilities.out index d0d0e63bf..0f29d0063 100644 --- a/src/test/regress/expected/multi_utilities.out +++ b/src/test/regress/expected/multi_utilities.out @@ -210,6 +210,45 @@ begin extract(epoch from clock_timestamp() - start_time); end $$ language plpgsql; +\c - - - :worker_2_port +CREATE MATERIALIZED VIEW prevcounts AS +SELECT analyze_count, vacuum_count FROM pg_stat_user_tables +WHERE relname='dustbunnies_990001'; +-- create function that sleeps until those counters increment +create function wait_for_stats() returns void as $$ +declare + start_time timestamptz := clock_timestamp(); + analyze_updated bool; + vacuum_updated bool; +begin + -- we don't want to wait forever; loop will exit after 10 seconds + for i in 1 .. 100 loop + + -- check to see if analyze has been updated + SELECT (st.analyze_count >= pc.analyze_count + 1) INTO analyze_updated + FROM pg_stat_user_tables AS st, pg_class AS cl, prevcounts AS pc + WHERE st.relname='dustbunnies_990001' AND cl.relname='dustbunnies_990001'; + + -- check to see if vacuum has been updated + SELECT (st.vacuum_count >= pc.vacuum_count + 1) INTO vacuum_updated + FROM pg_stat_user_tables AS st, pg_class AS cl, prevcounts AS pc + WHERE st.relname='dustbunnies_990001' AND cl.relname='dustbunnies_990001'; + + exit when analyze_updated or vacuum_updated; + + -- wait a little + perform pg_sleep(0.1); + + -- reset stats snapshot so we can test again + perform pg_stat_clear_snapshot(); + + end loop; + + -- report time waited in postmaster log (where it won't change test output) + raise log 'wait_for_stats delayed % seconds', + extract(epoch from clock_timestamp() - start_time); +end +$$ language plpgsql; -- run VACUUM and ANALYZE against the table on the master \c - - - :master_port VACUUM dustbunnies; @@ -317,12 +356,59 @@ WHERE tablename = 'dustbunnies_990002' ORDER BY attname; VACUUM; WARNING: not propagating VACUUM command to worker nodes HINT: Provide a specific table in order to VACUUM distributed tables. +-- check the current number of vacuum and analyze run on dustbunnies +SELECT run_command_on_workers($$SELECT wait_for_stats()$$); + run_command_on_workers +------------------------ + (localhost,57637,t,"") + (localhost,57638,t,"") +(2 rows) + +SELECT run_command_on_workers($$SELECT pg_stat_get_vacuum_count(tablename::regclass) from pg_tables where tablename LIKE 'dustbunnies_%' limit 1$$); + run_command_on_workers +------------------------ + (localhost,57637,t,3) + (localhost,57638,t,3) +(2 rows) + +SELECT run_command_on_workers($$SELECT pg_stat_get_analyze_count(tablename::regclass) from pg_tables where tablename LIKE 'dustbunnies_%' limit 1$$); + run_command_on_workers +------------------------ + (localhost,57637,t,3) + (localhost,57638,t,3) +(2 rows) + -- and warning when using targeted VACUUM without DDL propagation SET citus.enable_ddl_propagation to false; VACUUM dustbunnies; WARNING: not propagating VACUUM command to worker nodes HINT: Set citus.enable_ddl_propagation to true in order to send targeted VACUUM commands to worker nodes. +ANALYZE dustbunnies; +WARNING: not propagating ANALYZE command to worker nodes +HINT: Set citus.enable_ddl_propagation to true in order to send targeted ANALYZE commands to worker nodes. SET citus.enable_ddl_propagation to DEFAULT; +-- should not propagate the vacuum and analyze +SELECT run_command_on_workers($$SELECT wait_for_stats()$$); + run_command_on_workers +------------------------ + (localhost,57637,t,"") + (localhost,57638,t,"") +(2 rows) + +SELECT run_command_on_workers($$SELECT pg_stat_get_vacuum_count(tablename::regclass) from pg_tables where tablename LIKE 'dustbunnies_%' limit 1$$); + run_command_on_workers +------------------------ + (localhost,57637,t,3) + (localhost,57638,t,3) +(2 rows) + +SELECT run_command_on_workers($$SELECT pg_stat_get_analyze_count(tablename::regclass) from pg_tables where tablename LIKE 'dustbunnies_%' limit 1$$); + run_command_on_workers +------------------------ + (localhost,57637,t,3) + (localhost,57638,t,3) +(2 rows) + -- test worker_hash SELECT worker_hash(123); worker_hash diff --git a/src/test/regress/sql/multi_utilities.sql b/src/test/regress/sql/multi_utilities.sql index 0e48f695b..0e84a9226 100644 --- a/src/test/regress/sql/multi_utilities.sql +++ b/src/test/regress/sql/multi_utilities.sql @@ -145,6 +145,46 @@ begin end $$ language plpgsql; +\c - - - :worker_2_port +CREATE MATERIALIZED VIEW prevcounts AS +SELECT analyze_count, vacuum_count FROM pg_stat_user_tables +WHERE relname='dustbunnies_990001'; +-- create function that sleeps until those counters increment +create function wait_for_stats() returns void as $$ +declare + start_time timestamptz := clock_timestamp(); + analyze_updated bool; + vacuum_updated bool; +begin + -- we don't want to wait forever; loop will exit after 10 seconds + for i in 1 .. 100 loop + + -- check to see if analyze has been updated + SELECT (st.analyze_count >= pc.analyze_count + 1) INTO analyze_updated + FROM pg_stat_user_tables AS st, pg_class AS cl, prevcounts AS pc + WHERE st.relname='dustbunnies_990001' AND cl.relname='dustbunnies_990001'; + + -- check to see if vacuum has been updated + SELECT (st.vacuum_count >= pc.vacuum_count + 1) INTO vacuum_updated + FROM pg_stat_user_tables AS st, pg_class AS cl, prevcounts AS pc + WHERE st.relname='dustbunnies_990001' AND cl.relname='dustbunnies_990001'; + + exit when analyze_updated or vacuum_updated; + + -- wait a little + perform pg_sleep(0.1); + + -- reset stats snapshot so we can test again + perform pg_stat_clear_snapshot(); + + end loop; + + -- report time waited in postmaster log (where it won't change test output) + raise log 'wait_for_stats delayed % seconds', + extract(epoch from clock_timestamp() - start_time); +end +$$ language plpgsql; + -- run VACUUM and ANALYZE against the table on the master \c - - - :master_port VACUUM dustbunnies; @@ -209,11 +249,22 @@ WHERE tablename = 'dustbunnies_990002' ORDER BY attname; -- verify warning for unqualified VACUUM VACUUM; +-- check the current number of vacuum and analyze run on dustbunnies +SELECT run_command_on_workers($$SELECT wait_for_stats()$$); +SELECT run_command_on_workers($$SELECT pg_stat_get_vacuum_count(tablename::regclass) from pg_tables where tablename LIKE 'dustbunnies_%' limit 1$$); +SELECT run_command_on_workers($$SELECT pg_stat_get_analyze_count(tablename::regclass) from pg_tables where tablename LIKE 'dustbunnies_%' limit 1$$); + -- and warning when using targeted VACUUM without DDL propagation SET citus.enable_ddl_propagation to false; VACUUM dustbunnies; +ANALYZE dustbunnies; SET citus.enable_ddl_propagation to DEFAULT; +-- should not propagate the vacuum and analyze +SELECT run_command_on_workers($$SELECT wait_for_stats()$$); +SELECT run_command_on_workers($$SELECT pg_stat_get_vacuum_count(tablename::regclass) from pg_tables where tablename LIKE 'dustbunnies_%' limit 1$$); +SELECT run_command_on_workers($$SELECT pg_stat_get_analyze_count(tablename::regclass) from pg_tables where tablename LIKE 'dustbunnies_%' limit 1$$); + -- test worker_hash SELECT worker_hash(123); SELECT worker_hash('1997-08-08'::date);