Make citus_stat_tenants work with schema-based tenants. #6936

gokhangulbiz · 2023-05-23T14:17:48Z

DESCRIPTION: Enabling citus_stat_tenants to support schema-based tenants.

This pull request modifies the existing logic to enable tenant monitoring with schema-based tenants. The changes made are as follows:

If a query has a partitionKeyValue (which serves as a tenant key/identifier for distributed tables), Citus annotates the query with both the partitionKeyValue and colocationId. This allows for accurate tracking of the query.
If a query does not have a partitionKeyValue, but its colocationId belongs to a distributed schema, Citus annotates the query with only the colocationId. The tenant monitor can then easily look up the schema to determine if it's a distributed schema and make a decision on whether to track the query.

codecov · 2023-05-29T08:03:09Z

Codecov Report

Merging #6936 (91661a6) into main (5acbd73) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 91661a6 differs from pull request most recent head 4dc9d23. Consider uploading reports for the commit 4dc9d23 to get more accurate results

@@           Coverage Diff            @@
##             main    #6936    +/-   ##
========================================
  Coverage   93.27%   93.28%            
========================================
  Files         272      272            
  Lines       58137    57942   -195     
========================================
- Hits        54230    54049   -181     
+ Misses       3907     3893    -14

…tion key.

src/backend/distributed/utils/citus_stat_tenants.c

JelteF · 2023-06-07T07:33:30Z

src/backend/distributed/executor/local_executor.c

 */
 static void
 SetColocationIdAndPartitionKeyValueForTasks(List *taskList, Job *workerJob)
 {
-	if (workerJob->colocationId != 0 &&


I think we can keep the workerJob->colocationId != 0 check, might be a nice optimization for multi shard queries with many tasks.

Co-authored-by: Jelte Fennema <jelte.fennema@microsoft.com>

JelteF · 2023-06-09T10:41:00Z

src/backend/distributed/utils/citus_stat_tenants.c

+	/*
+	 * if tenantId is NULL, it must be a schema-based tenant and
+	 * we try to get the tenantId from the colocationId to lookup schema name and use it as a tenantId
+	 */
+	if (tenantId == NULL)
+	{
+		if (!IsTenantSchemaColocationGroup(colocationId))
+		{
+			return;
+		}
+
+	}


I don't think this check is needed anymore. We only annotate the query if with tenantId=NULL if IsTenantSchemaColocationGroup returned true. So no need to re-check it here again.

You're right, except for locally executed queries (ex: queries to reference tables) that directly follow the AttributeTask(..) flow without being annotated. We can move this check to local_executor. Let me know your thoughts.

src/test/regress/sql/citus_stat_tenants.sql

src/backend/distributed/executor/local_executor.c

JelteF · 2023-06-09T10:46:42Z

src/backend/distributed/utils/citus_stat_tenants.c

@@ -683,7 +740,8 @@ FindTenantStats(MultiTenantMonitor *monitor)
 	for (int i = 0; i < monitor->tenantCount; i++)
 	{
 		TenantStats *tenantStats = &monitor->tenants[i];
-		if (strcmp(tenantStats->tenantAttribute, AttributeToTenant) == 0 &&
+
+		if (strncmp(tenantStats->tenantAttribute, AttributeToTenant, MAX_TENANT_ATTRIBUTE_LENGTH) == 0 &&


Why was this change necessary?

I don't have a solid reason. I was just curious about its impact on performance. In any case, it's no longer necessary.

Co-authored-by: Jelte Fennema <jelte.fennema@microsoft.com>

…gulbiz/schema-based-tenant-monitoring

JelteF · 2023-06-12T08:33:01Z

src/test/regress/expected/adaptive_executor_repartition.out

@@ -173,6 +173,6 @@ DETAIL:  drop cascades to table ab
 drop cascades to table single_hash_repartition_first
 drop cascades to table single_hash_repartition_second
 drop cascades to table ref_table
-drop cascades to table ref_table_361397
+drop cascades to table ref_table_361399


can you instead fix this by running SET client_min_messages TO WARNING; before the DROP SCHEMA? That will make this diff is not necessary anymore.

…y output.

…gulbiz/schema-based-tenant-monitoring

Base automatically changed from schema-based-sharding-via-guc to main May 26, 2023 07:50

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch from e3e140d to d76e072 Compare May 29, 2023 07:49

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch from d76e072 to 4923ad4 Compare May 29, 2023 08:11

gokhangulbiz changed the title ~~Initial implementation of schema-based tenants integration to citus_s…~~ Make citus_stat_tenants work with schema-based tenants. May 29, 2023

gokhangulbiz marked this pull request as ready for review May 29, 2023 08:26

gokhangulbiz requested a review from JelteF May 29, 2023 08:27

gokhangulbiz added 6 commits June 5, 2023 16:56

Make ColocationIdGetTenantSchemaId accessable

b86bea2

Set colocationId even it has no partition key.

495c7e2

Set colocationId and partitionKeyValue for tasks even it has no parti…

d888966

…tion key.

Make tenant monitoring work with schema-based tenants

b9c4ca4

Add regression tests.

e847a22

Comments & indents

83b5f0e

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch from aeb2288 to 83b5f0e Compare June 5, 2023 14:06

Update tenants statistics annotations normalization

fea3b1c

JelteF reviewed Jun 7, 2023

View reviewed changes

src/backend/distributed/utils/citus_stat_tenants.c Outdated Show resolved Hide resolved

JelteF reviewed Jun 7, 2023

View reviewed changes

gokhangulbiz and others added 2 commits June 7, 2023 10:46

Indent

bcae49d

Co-authored-by: Jelte Fennema <jelte.fennema@microsoft.com>

Set colocationId and partitionKey if workerJob has a valid colocationId.

80a52a0

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch from 9e2b205 to 80a52a0 Compare June 7, 2023 07:57

gokhangulbiz added 4 commits June 7, 2023 14:49

Move schema lookup logic to UDF's

8ff5d36

Merge branch 'main' into gokhangulbiz/schema-based-tenant-monitoring

7241615

Merge branch 'main' into gokhangulbiz/schema-based-tenant-monitoring

e2ba016

Fix permissions for newly introduced udfs

50194fe

JelteF reviewed Jun 9, 2023

View reviewed changes

src/test/regress/sql/citus_stat_tenants.sql Show resolved Hide resolved

JelteF reviewed Jun 9, 2023

View reviewed changes

src/backend/distributed/executor/local_executor.c Outdated Show resolved Hide resolved

JelteF reviewed Jun 9, 2023

View reviewed changes

gokhangulbiz added 2 commits June 9, 2023 14:56

Merge branch 'main' into gokhangulbiz/schema-based-tenant-monitoring

6341c0a

Indent

95ebd12

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch 2 times, most recently from c1aaae8 to 0bc4ced Compare June 9, 2023 13:24

Fix downgrade sql scripts

56691d7

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch from 0bc4ced to 56691d7 Compare June 9, 2023 13:41

gokhangulbiz and others added 8 commits June 9, 2023 16:50

Merge branch 'main' into gokhangulbiz/schema-based-tenant-monitoring

ad0e7c3

magic number fix

a65ea36

Co-authored-by: Jelte Fennema <jelte.fennema@microsoft.com>

Add missing header

b712ffa

Add tests for single shard distributed tables

5eb6c76

Merge branch 'main' into gokhangulbiz/schema-based-tenant-monitoring

7b762cb

Merge branch 'main' of https://github.com/citusdata/citus into gokhan…

6284039

…gulbiz/schema-based-tenant-monitoring

Add null-check in FillTenantStatsHashkey

54ead77

Fix expected test output

9fca27b

JelteF reviewed Jun 12, 2023

View reviewed changes

Set client_min_messages to warning before drop schema to prevent flak…

dcfe045

…y output.

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch from 0585a36 to dcfe045 Compare June 12, 2023 09:02

Merge branch 'main' into gokhangulbiz/schema-based-tenant-monitoring

91661a6

gokhangulbiz requested a review from JelteF June 13, 2023 09:36

JelteF approved these changes Jun 13, 2023

View reviewed changes

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch from cc11493 to 10d42ca Compare June 13, 2023 10:49

gokhangulbiz enabled auto-merge (squash) June 13, 2023 10:52

gokhangulbiz disabled auto-merge June 13, 2023 10:53

Merge branch 'main' of https://github.com/citusdata/citus into gokhan…

4dc9d23

…gulbiz/schema-based-tenant-monitoring

gokhangulbiz force-pushed the gokhangulbiz/schema-based-tenant-monitoring branch from 10d42ca to 4dc9d23 Compare June 13, 2023 10:56

gokhangulbiz merged commit e0ccd15 into main Jun 13, 2023
111 checks passed

gokhangulbiz deleted the gokhangulbiz/schema-based-tenant-monitoring branch June 13, 2023 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make citus_stat_tenants work with schema-based tenants. #6936

Make citus_stat_tenants work with schema-based tenants. #6936

gokhangulbiz commented May 23, 2023 •

edited

codecov bot commented May 29, 2023 •

edited

JelteF Jun 7, 2023 •

edited

JelteF Jun 9, 2023

gokhangulbiz Jun 11, 2023

JelteF Jun 9, 2023

gokhangulbiz Jun 11, 2023

JelteF Jun 12, 2023

Make citus_stat_tenants work with schema-based tenants. #6936

Make citus_stat_tenants work with schema-based tenants. #6936

Conversation

gokhangulbiz commented May 23, 2023 • edited

codecov bot commented May 29, 2023 • edited

Codecov Report

JelteF Jun 7, 2023 • edited

Choose a reason for hiding this comment

JelteF Jun 9, 2023

Choose a reason for hiding this comment

gokhangulbiz Jun 11, 2023

Choose a reason for hiding this comment

JelteF Jun 9, 2023

Choose a reason for hiding this comment

gokhangulbiz Jun 11, 2023

Choose a reason for hiding this comment

JelteF Jun 12, 2023

Choose a reason for hiding this comment

gokhangulbiz commented May 23, 2023 •

edited

codecov bot commented May 29, 2023 •

edited

JelteF Jun 7, 2023 •

edited