Add support for disabling hash aggregate with hll #65

velioglu · 2018-12-05T08:29:31Z

This PR adds support for disabling hash aggregate for hll aggregate functions. In order to disable cost of the path with hash aggregate is maximized, so the planner is forced to select group aggregate. Client can utilize this functionality by setting hll.disable_hashagg to ON at session level.

Remaining items:

Ensure that each version passes the test
Add more tests with (especially with hll_add_agg)
Check the support with Citus
Get function oids from cache

byucesoy

I had a very quick look. I'll have a more detailed look after the caching part is completed.

src/hll.c

mtuncer · 2018-12-05T15:16:20Z

src/hll.c

+RegisterHLLConfigVariables(void)
+{
+	DefineCustomBoolVariable(
+		"hll.disable_hashagg",


somehow enable_hashagg felt better here, postgres also uses it.

mtuncer · 2018-12-10T13:24:16Z

src/hll.c

+	foreach(cacheEntryCell, hllAggregateCache)
+	{
+		HllAggregateCacheEntry *cacheEntry = (HllAggregateCacheEntry *)lfirst(cacheEntryCell);
+ 		if (strcmp(aggregateName, cacheEntry->aggregateName) == 0 &&


sequential scan on a list + text comparison. Could this get us into trouble ? Maybe compare argument count first

mtuncer · 2018-12-10T13:25:53Z

src/hll.c

+ 		oldContext = MemoryContextSwitchTo(CacheMemoryContext);
+ 		matchingCacheEntry = palloc0(sizeof(HllAggregateCacheEntry));
+		matchingCacheEntry->aggregateId = aggregateId;
+		matchingCacheEntry->aggregateName = aggregateName;


we are storing a pointer to char whose lifetime is not know. We should copy instead.

it appears they are constants, then do we need to store text at all ? can we store an integer perhaps so that we can easily check the id ?

mtuncer · 2018-12-10T13:30:32Z

src/hll.c

+	heap_close(rel, AccessShareLock);
+
+	return result;
+	/* *INDENT-ON* */


INDENT-ON ?

src/hll.c

mtuncer

Minor requests

Please re-evaluate function cache design

mtuncer

🚢

mtuncer · 2018-12-10T14:22:58Z

spoke too soon

Travis fails left and right, you will need to add hll to shared_preload_libraries
see https://github.com/citusdata/tools/blob/develop/travis/config_and_start_cluster#L17

mtuncer · 2018-12-10T14:23:35Z

accidentally closed the PR

byucesoy · 2018-12-11T08:20:03Z

src/hll.c

+	HLL_UNION_AGG_ID = 1
+} HllAggregationId;
+
+bool ForceGroupagg = false;


ForceGroupagg -> ForceGroupAgg

src/hll.c

byucesoy · 2018-12-11T08:43:52Z

src/hll.c

+	foreach(cacheEntryCell, hllAggregateCache)
+	{
+		HllAggregateCacheEntry *cacheEntry = (HllAggregateCacheEntry *)lfirst(cacheEntryCell);
+		if (argumentCount == cacheEntry->argumentCount &&


This might be slightly OCD but can we change the order to be like this;

if (cacheEntry->argumentCount == argumentCount && cacheEntry->hllAggregationId == hllAggregationId)

Personally, I would love to have hllAggregationId comparison first, but with the other comment below, having count comparison first makes more sense.

byucesoy · 2018-12-11T08:50:30Z

src/hll.c

+		Oid hllId = get_extension_oid(HLL_EXTENSION_NAME, false);
+		Oid hllSchemaOid = get_extension_schema(hllId);
+		const char *hllSchemaName = get_namespace_name(hllSchemaOid);
+		char *aggregateName = hllAggregationId == HLL_ADD_AGG_ID ?


In the end, we are still using aggregate names, so I think we don't need an extra level of abstraction (i.e. enum and aggregation ids). Without the enum, the code would be much simpler and only drawback I can see is we had to do string comparison in the loop above, which is mostly OK especially if we first compare argument counts.

byucesoy · 2018-12-11T10:07:46Z

src/hll.c

+// Support disabling hash aggregation functionality for PG > 9.6
+#if PG_VERSION_NUM >= 90600
+
+#define HLL_EXTENSION_NAME "hll"


No need for HLL prefix, this is HLL extension anyway. We don't use HLL keyword anywhere in the codebase except functions exposed to user space.

src/hll.c

byucesoy · 2018-12-11T10:15:02Z

src/hll.c

+								   RelOptInfo *input_rel, RelOptInfo *output_rel)
+#endif
+{
+	Oid hllId = InvalidOid;


hllId -> extension_id

byucesoy · 2018-12-11T10:21:19Z

src/hll.c

+		{
+			Aggref *aggref = (Aggref *) var;
+			int argCount = list_length(aggref->args);
+			Oid addFunctionId = LookupAggregatorOidByName(HLL_ADD_AGG_ID, argCount);


I'm starting to think, that it would be better to have two static variables (like add_aggregate_oid[] and union_aggregate_id) for function oids which are populated at pg_init. This would save us from all caching logic.

mtuncer · 2018-12-11T13:56:18Z

sql/disable_hashagg.sql

+HAVING hll_cardinality(hll_union_agg(sd)) > 1;
+
+-- Test hll_add_agg, first set work_mem to 256MB in order to use hash aggregate
+-- befire forcing group aggregate


befire -> before

mtuncer

🚢 after minor changes

mtuncer · 2018-12-11T13:58:34Z

expected/disable_hashagg.out

+-- ----------------------------------------------------------------
+-- Regression tests for disabling hash aggregation
+-- ----------------------------------------------------------------
+SELECT hll_set_output_version(1);


all disable hash_agg expectations look the same. it would be better if we put some sort of PG version information that caused us to make this differentiation

mtuncer · 2018-12-11T13:59:55Z

src/hll.c

+								   RelOptInfo *input_rel, RelOptInfo *output_rel)
+#endif
+{
+	Oid hllId = InvalidOid;


mtuncer · 2018-12-11T14:01:28Z

src/hll.c

+
+	/* Initialize HLL_ADD_AGG with different signatures */
+	aggregateName = ADD_AGG_NAME;
+	for ( ; addAggArgumentCounter < HLL_AGGREGATE_COUNT ; addAggArgumentCounter++)


odd use of for loop

marcocitus · 2018-12-11T17:37:19Z

src/hll.c

+
+			if(HllAggregateOid(aggref->aggfnoid))
+			{
+				path->total_cost = INT_MAX;


INT_MAX is a pretty arbitrary value.

total_cost is not an int. If it was then setting it to the max would be a bad idea because it would likely overflow when other costs are added, now it's just arbitrary, which is probably fine?

metdos assigned mtuncer Dec 5, 2018

byucesoy reviewed Dec 5, 2018

View reviewed changes

src/hll.c Outdated Show resolved Hide resolved

src/hll.c Outdated Show resolved Hide resolved

mtuncer reviewed Dec 5, 2018

View reviewed changes

src/hll.c Show resolved Hide resolved

mtuncer reviewed Dec 5, 2018

View reviewed changes

src/hll.c Outdated Show resolved Hide resolved

mtuncer reviewed Dec 5, 2018

View reviewed changes

src/hll.c Outdated Show resolved Hide resolved

mtuncer reviewed Dec 5, 2018

View reviewed changes

velioglu force-pushed the disable_hashagg branch from b4e251a to 90e9cbd Compare December 10, 2018 12:54

mtuncer reviewed Dec 10, 2018

View reviewed changes

src/hll.c Outdated

heap_close(rel, AccessShareLock);

return result;

/* *INDENT-ON* */

Copy link

Member

mtuncer Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INDENT-ON ?

mtuncer reviewed Dec 10, 2018

View reviewed changes

src/hll.c Show resolved Hide resolved

mtuncer requested changes Dec 10, 2018

View reviewed changes

mtuncer approved these changes Dec 10, 2018

View reviewed changes

mtuncer closed this Dec 10, 2018

mtuncer reopened this Dec 10, 2018

velioglu force-pushed the disable_hashagg branch 2 times, most recently from a321592 to d22970b Compare December 10, 2018 19:31

byucesoy suggested changes Dec 11, 2018

View reviewed changes

byucesoy reviewed Dec 11, 2018

View reviewed changes

src/hll.c Show resolved Hide resolved

byucesoy reviewed Dec 11, 2018

View reviewed changes

src/hll.c Show resolved Hide resolved

byucesoy suggested changes Dec 11, 2018

View reviewed changes

velioglu force-pushed the disable_hashagg branch 2 times, most recently from 62615dd to 535c945 Compare December 11, 2018 13:55

mtuncer reviewed Dec 11, 2018

View reviewed changes

velioglu force-pushed the disable_hashagg branch from 535c945 to a4a5379 Compare December 11, 2018 13:57

mtuncer approved these changes Dec 11, 2018

View reviewed changes

velioglu force-pushed the disable_hashagg branch from 02d95c0 to 9dc4c91 Compare December 11, 2018 17:08

Add support for disabling hash aggregate with hll

9dc4c91

velioglu merged commit b536369 into master Dec 11, 2018

marcocitus reviewed Dec 11, 2018

View reviewed changes

byucesoy deleted the disable_hashagg branch September 2, 2019 10:02

Add support for disabling hash aggregate with hll #65

Add support for disabling hash aggregate with hll #65

Uh oh!

Conversation

velioglu commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

byucesoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtuncer left a comment

Choose a reason for hiding this comment

Uh oh!

mtuncer left a comment

Choose a reason for hiding this comment

Uh oh!

mtuncer commented Dec 10, 2018

Uh oh!

mtuncer commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtuncer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

velioglu commented Dec 5, 2018 •

edited

Loading

mtuncer commented Dec 10, 2018 •

edited

Loading