From 6ed64d06ea22d265c060cf8ce42ec77fc441b986 Mon Sep 17 00:00:00 2001 From: Metin Doslu Date: Wed, 27 Jun 2018 14:28:49 +0300 Subject: [PATCH 1/4] Respect enable_hashagg in the master planner for group by --- .../planner/multi_master_planner.c | 43 +++++++++++++++++-- src/test/regress/expected/multi_explain.out | 20 +++++++++ src/test/regress/expected/multi_explain_0.out | 20 +++++++++ src/test/regress/sql/multi_explain.sql | 8 ++++ 4 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/backend/distributed/planner/multi_master_planner.c b/src/backend/distributed/planner/multi_master_planner.c index 6b4be984f..b1cbbc631 100644 --- a/src/backend/distributed/planner/multi_master_planner.c +++ b/src/backend/distributed/planner/multi_master_planner.c @@ -21,6 +21,7 @@ #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" #include "optimizer/clauses.h" +#include "optimizer/cost.h" #include "optimizer/planmain.h" #include "optimizer/tlist.h" #include "optimizer/var.h" @@ -128,13 +129,47 @@ BuildAggregatePlan(Query *masterQuery, Plan *subPlan) /* if we have grouping, then initialize appropriate information */ if (groupColumnCount > 0) { - if (!grouping_is_hashable(groupColumnList)) + bool groupingIsHashable = grouping_is_hashable(groupColumnList); + bool groupingIsSortable = grouping_is_sortable(groupColumnList); + + if (!groupingIsHashable && !groupingIsSortable) { - ereport(ERROR, (errmsg("grouped column list cannot be hashed"))); + ereport(ERROR, (errmsg("grouped column list cannot be hashed or sorted"))); } - /* switch to hashed aggregate strategy to allow grouping */ - aggregateStrategy = AGG_HASHED; + /* + * Postgres hash aggregate strategy does not support distinct aggregates + * in group and order by with aggregate operations. + * see nodeAgg.c:build_pertrans_for_aggref(). In that case we use + * sorted agg strategy, otherwise we use hash strategy. + */ + if (!enable_hashagg || !groupingIsHashable) + { + char *messageHint = NULL; + if (!enable_hashagg && groupingIsHashable) + { + messageHint = "Consider setting enable_hashagg to on."; + } + + if (!groupingIsSortable) + { + ereport(ERROR, (errmsg("grouped column list must cannot be sorted"), + errdetail("Having a distinct aggregate requires " + "grouped column list to be sortable."), + messageHint ? errhint("%s", messageHint) : 0)); + } + + aggregateStrategy = AGG_SORTED; +#if (PG_VERSION_NUM >= 90600) + subPlan = (Plan *) make_sort_from_sortclauses(groupColumnList, subPlan); +#else + subPlan = (Plan *) make_sort_from_sortclauses(NULL, groupColumnList, subPlan); +#endif + } + else + { + aggregateStrategy = AGG_HASHED; + } /* get column indexes that are being grouped */ groupColumnIdArray = extract_grouping_cols(groupColumnList, subPlan->targetlist); diff --git a/src/test/regress/expected/multi_explain.out b/src/test/regress/expected/multi_explain.out index 2d8193b6a..dcb0564ff 100644 --- a/src/test/regress/expected/multi_explain.out +++ b/src/test/regress/expected/multi_explain.out @@ -686,3 +686,23 @@ Custom Scan (Citus Router) -> Seq Scan on explain_table_570001 explain_table Filter: (id = 1) ROLLBACK; +-- Test disable hash aggregate +SET enable_hashagg TO off; +EXPLAIN (COSTS FALSE, FORMAT TEXT) + SELECT l_quantity, count(*) count_quantity FROM lineitem + GROUP BY l_quantity ORDER BY count_quantity, l_quantity; +Sort + Sort Key: COALESCE((pg_catalog.sum((COALESCE((pg_catalog.sum(remote_scan.count_quantity))::bigint, '0'::bigint))))::bigint, '0'::bigint), remote_scan.l_quantity + -> GroupAggregate + Group Key: remote_scan.l_quantity + -> Sort + Sort Key: remote_scan.l_quantity + -> Custom Scan (Citus Real-Time) + Task Count: 8 + Tasks Shown: One of 8 + -> Task + Node: host=localhost port=57637 dbname=regression + -> HashAggregate + Group Key: l_quantity + -> Seq Scan on lineitem_290001 lineitem +SET enable_hashagg TO on; diff --git a/src/test/regress/expected/multi_explain_0.out b/src/test/regress/expected/multi_explain_0.out index 10798f526..110bf77a3 100644 --- a/src/test/regress/expected/multi_explain_0.out +++ b/src/test/regress/expected/multi_explain_0.out @@ -657,3 +657,23 @@ Custom Scan (Citus Router) -> Seq Scan on explain_table_570001 explain_table Filter: (id = 1) ROLLBACK; +-- Test disable hash aggregate +SET enable_hashagg TO off; +EXPLAIN (COSTS FALSE, FORMAT TEXT) + SELECT l_quantity, count(*) count_quantity FROM lineitem + GROUP BY l_quantity ORDER BY count_quantity, l_quantity; +Sort + Sort Key: COALESCE((pg_catalog.sum((COALESCE((pg_catalog.sum(remote_scan.count_quantity))::bigint, '0'::bigint))))::bigint, '0'::bigint), remote_scan.l_quantity + -> GroupAggregate + Group Key: remote_scan.l_quantity + -> Sort + Sort Key: remote_scan.l_quantity + -> Custom Scan (Citus Real-Time) + Task Count: 8 + Tasks Shown: One of 8 + -> Task + Node: host=localhost port=57637 dbname=regression + -> HashAggregate + Group Key: l_quantity + -> Seq Scan on lineitem_290001 lineitem +SET enable_hashagg TO on; diff --git a/src/test/regress/sql/multi_explain.sql b/src/test/regress/sql/multi_explain.sql index e744b1f31..ccbdcc49c 100644 --- a/src/test/regress/sql/multi_explain.sql +++ b/src/test/regress/sql/multi_explain.sql @@ -247,3 +247,11 @@ ALTER TABLE explain_table ADD COLUMN value int; EXPLAIN (COSTS FALSE) SELECT value FROM explain_table WHERE id = 1; ROLLBACK; + +-- Test disable hash aggregate +SET enable_hashagg TO off; +EXPLAIN (COSTS FALSE, FORMAT TEXT) + SELECT l_quantity, count(*) count_quantity FROM lineitem + GROUP BY l_quantity ORDER BY count_quantity, l_quantity; + +SET enable_hashagg TO on; From 40a606cbbba00d0c385e88534852a3eca7308932 Mon Sep 17 00:00:00 2001 From: velioglu Date: Fri, 6 Jul 2018 12:31:51 +0300 Subject: [PATCH 2/4] Fix tests --- src/test/regress/expected/multi_explain_0.out | 2 +- .../expected/multi_index_statements.out | 68 +++++++++---------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/src/test/regress/expected/multi_explain_0.out b/src/test/regress/expected/multi_explain_0.out index 110bf77a3..90b294a65 100644 --- a/src/test/regress/expected/multi_explain_0.out +++ b/src/test/regress/expected/multi_explain_0.out @@ -663,7 +663,7 @@ EXPLAIN (COSTS FALSE, FORMAT TEXT) SELECT l_quantity, count(*) count_quantity FROM lineitem GROUP BY l_quantity ORDER BY count_quantity, l_quantity; Sort - Sort Key: COALESCE((pg_catalog.sum((COALESCE((pg_catalog.sum(remote_scan.count_quantity))::bigint, '0'::bigint))))::bigint, '0'::bigint), remote_scan.l_quantity + Sort Key: COALESCE((sum((COALESCE((sum(remote_scan.count_quantity))::bigint, '0'::bigint))))::bigint, '0'::bigint), remote_scan.l_quantity -> GroupAggregate Group Key: remote_scan.l_quantity -> Sort diff --git a/src/test/regress/expected/multi_index_statements.out b/src/test/regress/expected/multi_index_statements.out index cc2ac53bf..b9f10e20d 100644 --- a/src/test/regress/expected/multi_index_statements.out +++ b/src/test/regress/expected/multi_index_statements.out @@ -98,23 +98,23 @@ CREATE INDEX CONCURRENTLY ON local_table(id); DROP TABLE local_table; -- Verify that all indexes got created on the master node and one of the workers SELECT * FROM pg_indexes WHERE tablename = 'lineitem' or tablename like 'index_test_%' ORDER BY indexname; - schemaname | tablename | indexname | tablespace | indexdef -------------+------------------+------------------------------------+------------+--------------------------------------------------------------------------------------------------------------------- - public | index_test_hash | index_test_hash_index_a | | CREATE UNIQUE INDEX index_test_hash_index_a ON index_test_hash USING btree (a) - public | index_test_hash | index_test_hash_index_a_b | | CREATE UNIQUE INDEX index_test_hash_index_a_b ON index_test_hash USING btree (a, b) - public | index_test_hash | index_test_hash_index_a_b_partial | | CREATE UNIQUE INDEX index_test_hash_index_a_b_partial ON index_test_hash USING btree (a, b) WHERE (c IS NOT NULL) - public | index_test_range | index_test_range_index_a | | CREATE UNIQUE INDEX index_test_range_index_a ON index_test_range USING btree (a) - public | index_test_range | index_test_range_index_a_b | | CREATE UNIQUE INDEX index_test_range_index_a_b ON index_test_range USING btree (a, b) - public | index_test_range | index_test_range_index_a_b_partial | | CREATE UNIQUE INDEX index_test_range_index_a_b_partial ON index_test_range USING btree (a, b) WHERE (c IS NOT NULL) - public | lineitem | lineitem_colref_index | | CREATE INDEX lineitem_colref_index ON lineitem USING btree (record_ne(lineitem.*, NULL::record)) - public | lineitem | lineitem_concurrently_index | | CREATE INDEX lineitem_concurrently_index ON lineitem USING btree (l_orderkey) - public | lineitem | lineitem_orderkey_hash_index | | CREATE INDEX lineitem_orderkey_hash_index ON lineitem USING hash (l_partkey) - public | lineitem | lineitem_orderkey_index | | CREATE INDEX lineitem_orderkey_index ON lineitem USING btree (l_orderkey) - public | lineitem | lineitem_orderkey_index_new | | CREATE INDEX lineitem_orderkey_index_new ON lineitem USING btree (l_orderkey) - public | lineitem | lineitem_partial_index | | CREATE INDEX lineitem_partial_index ON lineitem USING btree (l_shipdate) WHERE (l_shipdate < '01-01-1995'::date) - public | lineitem | lineitem_partkey_desc_index | | CREATE INDEX lineitem_partkey_desc_index ON lineitem USING btree (l_partkey DESC) - public | lineitem | lineitem_pkey | | CREATE UNIQUE INDEX lineitem_pkey ON lineitem USING btree (l_orderkey, l_linenumber) - public | lineitem | lineitem_time_index | | CREATE INDEX lineitem_time_index ON lineitem USING btree (l_shipdate) + schemaname | tablename | indexname | tablespace | indexdef +------------+------------------+------------------------------------+------------+---------------------------------------------------------------------------------------------------------------------------- + public | index_test_hash | index_test_hash_index_a | | CREATE UNIQUE INDEX index_test_hash_index_a ON public.index_test_hash USING btree (a) + public | index_test_hash | index_test_hash_index_a_b | | CREATE UNIQUE INDEX index_test_hash_index_a_b ON public.index_test_hash USING btree (a, b) + public | index_test_hash | index_test_hash_index_a_b_partial | | CREATE UNIQUE INDEX index_test_hash_index_a_b_partial ON public.index_test_hash USING btree (a, b) WHERE (c IS NOT NULL) + public | index_test_range | index_test_range_index_a | | CREATE UNIQUE INDEX index_test_range_index_a ON public.index_test_range USING btree (a) + public | index_test_range | index_test_range_index_a_b | | CREATE UNIQUE INDEX index_test_range_index_a_b ON public.index_test_range USING btree (a, b) + public | index_test_range | index_test_range_index_a_b_partial | | CREATE UNIQUE INDEX index_test_range_index_a_b_partial ON public.index_test_range USING btree (a, b) WHERE (c IS NOT NULL) + public | lineitem | lineitem_colref_index | | CREATE INDEX lineitem_colref_index ON public.lineitem USING btree (record_ne(lineitem.*, NULL::record)) + public | lineitem | lineitem_concurrently_index | | CREATE INDEX lineitem_concurrently_index ON public.lineitem USING btree (l_orderkey) + public | lineitem | lineitem_orderkey_hash_index | | CREATE INDEX lineitem_orderkey_hash_index ON public.lineitem USING hash (l_partkey) + public | lineitem | lineitem_orderkey_index | | CREATE INDEX lineitem_orderkey_index ON public.lineitem USING btree (l_orderkey) + public | lineitem | lineitem_orderkey_index_new | | CREATE INDEX lineitem_orderkey_index_new ON public.lineitem USING btree (l_orderkey) + public | lineitem | lineitem_partial_index | | CREATE INDEX lineitem_partial_index ON public.lineitem USING btree (l_shipdate) WHERE (l_shipdate < '01-01-1995'::date) + public | lineitem | lineitem_partkey_desc_index | | CREATE INDEX lineitem_partkey_desc_index ON public.lineitem USING btree (l_partkey DESC) + public | lineitem | lineitem_pkey | | CREATE UNIQUE INDEX lineitem_pkey ON public.lineitem USING btree (l_orderkey, l_linenumber) + public | lineitem | lineitem_time_index | | CREATE INDEX lineitem_time_index ON public.lineitem USING btree (l_shipdate) (15 rows) \c - - - :worker_1_port @@ -175,23 +175,23 @@ CREATE INDEX ON lineitem (l_orderkey); ERROR: creating index without a name on a distributed table is currently unsupported -- Verify that none of failed indexes got created on the master node SELECT * FROM pg_indexes WHERE tablename = 'lineitem' or tablename like 'index_test_%' ORDER BY indexname; - schemaname | tablename | indexname | tablespace | indexdef -------------+------------------+------------------------------------+------------+--------------------------------------------------------------------------------------------------------------------- - public | index_test_hash | index_test_hash_index_a | | CREATE UNIQUE INDEX index_test_hash_index_a ON index_test_hash USING btree (a) - public | index_test_hash | index_test_hash_index_a_b | | CREATE UNIQUE INDEX index_test_hash_index_a_b ON index_test_hash USING btree (a, b) - public | index_test_hash | index_test_hash_index_a_b_partial | | CREATE UNIQUE INDEX index_test_hash_index_a_b_partial ON index_test_hash USING btree (a, b) WHERE (c IS NOT NULL) - public | index_test_range | index_test_range_index_a | | CREATE UNIQUE INDEX index_test_range_index_a ON index_test_range USING btree (a) - public | index_test_range | index_test_range_index_a_b | | CREATE UNIQUE INDEX index_test_range_index_a_b ON index_test_range USING btree (a, b) - public | index_test_range | index_test_range_index_a_b_partial | | CREATE UNIQUE INDEX index_test_range_index_a_b_partial ON index_test_range USING btree (a, b) WHERE (c IS NOT NULL) - public | lineitem | lineitem_colref_index | | CREATE INDEX lineitem_colref_index ON lineitem USING btree (record_ne(lineitem.*, NULL::record)) - public | lineitem | lineitem_concurrently_index | | CREATE INDEX lineitem_concurrently_index ON lineitem USING btree (l_orderkey) - public | lineitem | lineitem_orderkey_hash_index | | CREATE INDEX lineitem_orderkey_hash_index ON lineitem USING hash (l_partkey) - public | lineitem | lineitem_orderkey_index | | CREATE INDEX lineitem_orderkey_index ON lineitem USING btree (l_orderkey) - public | lineitem | lineitem_orderkey_index_new | | CREATE INDEX lineitem_orderkey_index_new ON lineitem USING btree (l_orderkey) - public | lineitem | lineitem_partial_index | | CREATE INDEX lineitem_partial_index ON lineitem USING btree (l_shipdate) WHERE (l_shipdate < '01-01-1995'::date) - public | lineitem | lineitem_partkey_desc_index | | CREATE INDEX lineitem_partkey_desc_index ON lineitem USING btree (l_partkey DESC) - public | lineitem | lineitem_pkey | | CREATE UNIQUE INDEX lineitem_pkey ON lineitem USING btree (l_orderkey, l_linenumber) - public | lineitem | lineitem_time_index | | CREATE INDEX lineitem_time_index ON lineitem USING btree (l_shipdate) + schemaname | tablename | indexname | tablespace | indexdef +------------+------------------+------------------------------------+------------+---------------------------------------------------------------------------------------------------------------------------- + public | index_test_hash | index_test_hash_index_a | | CREATE UNIQUE INDEX index_test_hash_index_a ON public.index_test_hash USING btree (a) + public | index_test_hash | index_test_hash_index_a_b | | CREATE UNIQUE INDEX index_test_hash_index_a_b ON public.index_test_hash USING btree (a, b) + public | index_test_hash | index_test_hash_index_a_b_partial | | CREATE UNIQUE INDEX index_test_hash_index_a_b_partial ON public.index_test_hash USING btree (a, b) WHERE (c IS NOT NULL) + public | index_test_range | index_test_range_index_a | | CREATE UNIQUE INDEX index_test_range_index_a ON public.index_test_range USING btree (a) + public | index_test_range | index_test_range_index_a_b | | CREATE UNIQUE INDEX index_test_range_index_a_b ON public.index_test_range USING btree (a, b) + public | index_test_range | index_test_range_index_a_b_partial | | CREATE UNIQUE INDEX index_test_range_index_a_b_partial ON public.index_test_range USING btree (a, b) WHERE (c IS NOT NULL) + public | lineitem | lineitem_colref_index | | CREATE INDEX lineitem_colref_index ON public.lineitem USING btree (record_ne(lineitem.*, NULL::record)) + public | lineitem | lineitem_concurrently_index | | CREATE INDEX lineitem_concurrently_index ON public.lineitem USING btree (l_orderkey) + public | lineitem | lineitem_orderkey_hash_index | | CREATE INDEX lineitem_orderkey_hash_index ON public.lineitem USING hash (l_partkey) + public | lineitem | lineitem_orderkey_index | | CREATE INDEX lineitem_orderkey_index ON public.lineitem USING btree (l_orderkey) + public | lineitem | lineitem_orderkey_index_new | | CREATE INDEX lineitem_orderkey_index_new ON public.lineitem USING btree (l_orderkey) + public | lineitem | lineitem_partial_index | | CREATE INDEX lineitem_partial_index ON public.lineitem USING btree (l_shipdate) WHERE (l_shipdate < '01-01-1995'::date) + public | lineitem | lineitem_partkey_desc_index | | CREATE INDEX lineitem_partkey_desc_index ON public.lineitem USING btree (l_partkey DESC) + public | lineitem | lineitem_pkey | | CREATE UNIQUE INDEX lineitem_pkey ON public.lineitem USING btree (l_orderkey, l_linenumber) + public | lineitem | lineitem_time_index | | CREATE INDEX lineitem_time_index ON public.lineitem USING btree (l_shipdate) (15 rows) -- From 8723a89b0377510bbb6120db06582c70a3f0ebd1 Mon Sep 17 00:00:00 2001 From: velioglu Date: Fri, 6 Jul 2018 13:50:40 +0300 Subject: [PATCH 3/4] Update tools version to pass travis --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 10f6b0c45..5397e5d69 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,7 +14,7 @@ env: - PGVERSION=9.5 - PGVERSION=9.6 before_install: - - git clone -b v0.6.1 --depth 1 https://github.com/citusdata/tools.git + - git clone -b v0.7.6 --depth 1 https://github.com/citusdata/tools.git - sudo make -C tools install - setup_apt - curl https://install.citusdata.com/community/deb.sh | sudo bash From c7213dd377e6524f18bc0a345b442f19ab96bae9 Mon Sep 17 00:00:00 2001 From: velioglu Date: Fri, 6 Jul 2018 13:57:42 +0300 Subject: [PATCH 4/4] Make citus_indent happy --- src/backend/distributed/planner/multi_explain.c | 8 ++++---- src/backend/distributed/planner/shard_pruning.c | 12 ++++++------ .../distributed/transaction/transaction_management.c | 8 ++++---- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/backend/distributed/planner/multi_explain.c b/src/backend/distributed/planner/multi_explain.c index 52d2e10dc..7ad46d131 100644 --- a/src/backend/distributed/planner/multi_explain.c +++ b/src/backend/distributed/planner/multi_explain.c @@ -492,26 +492,26 @@ BuildRemoteExplainQuery(char *queryString, ExplainState *es) case EXPLAIN_FORMAT_XML: { formatStr = "XML"; + break; } - break; case EXPLAIN_FORMAT_JSON: { formatStr = "JSON"; + break; } - break; case EXPLAIN_FORMAT_YAML: { formatStr = "YAML"; + break; } - break; default: { formatStr = "TEXT"; + break; } - break; } appendStringInfo(explainQuery, diff --git a/src/backend/distributed/planner/shard_pruning.c b/src/backend/distributed/planner/shard_pruning.c index 90babc333..7d32ad09e 100644 --- a/src/backend/distributed/planner/shard_pruning.c +++ b/src/backend/distributed/planner/shard_pruning.c @@ -632,8 +632,8 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla prune->lessConsts = constantClause; } matchedOp = true; + break; } - break; case BTLessEqualStrategyNumber: { @@ -645,8 +645,8 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla prune->lessEqualConsts = constantClause; } matchedOp = true; + break; } - break; case BTEqualStrategyNumber: { @@ -662,8 +662,8 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla prune->evaluatesToFalse = true; } matchedOp = true; + break; } - break; case BTGreaterEqualStrategyNumber: { @@ -676,8 +676,8 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla prune->greaterEqualConsts = constantClause; } matchedOp = true; + break; } - break; case BTGreaterStrategyNumber: { @@ -689,15 +689,15 @@ AddPartitionKeyRestrictionToInstance(ClauseWalkerContext *context, OpExpr *opCla prune->greaterConsts = constantClause; } matchedOp = true; + break; } - break; case ROWCOMPARE_NE: { /* TODO: could add support for this, if we feel like it */ matchedOp = false; + break; } - break; default: Assert(false); diff --git a/src/backend/distributed/transaction/transaction_management.c b/src/backend/distributed/transaction/transaction_management.c index a0d14d9b0..3603cd0c1 100644 --- a/src/backend/distributed/transaction/transaction_management.c +++ b/src/backend/distributed/transaction/transaction_management.c @@ -168,8 +168,8 @@ CoordinatedTransactionCallback(XactEvent event, void *arg) XactModificationLevel = XACT_MODIFICATION_NONE; dlist_init(&InProgressTransactions); CoordinatedTransactionUses2PC = false; + break; } - break; case XACT_EVENT_ABORT: { @@ -204,8 +204,8 @@ CoordinatedTransactionCallback(XactEvent event, void *arg) dlist_init(&InProgressTransactions); CoordinatedTransactionUses2PC = false; subXactAbortAttempted = false; + break; } - break; case XACT_EVENT_PARALLEL_COMMIT: case XACT_EVENT_PARALLEL_ABORT: @@ -270,8 +270,8 @@ CoordinatedTransactionCallback(XactEvent event, void *arg) * committed. This handles failure at COMMIT/PREPARE time. */ PostCommitMarkFailedShardPlacements(CoordinatedTransactionUses2PC); + break; } - break; case XACT_EVENT_PARALLEL_PRE_COMMIT: case XACT_EVENT_PRE_PREPARE: @@ -282,8 +282,8 @@ CoordinatedTransactionCallback(XactEvent event, void *arg) errmsg("cannot use 2PC in transactions involving " "multiple servers"))); } + break; } - break; } }