Add schema tracking support for UDFs #15705

systay · 2024-04-12T07:08:05Z

Description

Adds the capability of vttablets to notice that aggregate UDFs have been added to a MySQL system.
We need to track these to do planning correctly.

Related Issue(s)

Bug Report: User Defined Functions Fails to execute #15711

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Andres Taylor <andres@planetscale.com>

vitess-bot · 2024-04-12T07:08:07Z

harshit-gangal · 2024-04-15T09:10:55Z

Website Documentation: vitessio/website#1726

Signed-off-by: Andres Taylor <andres@planetscale.com>

…gate functions Signed-off-by: Harshit Gangal <harshit@planetscale.com>

… vtgate enableUDFs flag to keep it disabled by default Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

GuptaManan100

Rest LGTM!

go/vt/vtgate/vindexes/vschema.go

codecov · 2024-04-15T10:40:35Z

Codecov Report

Attention: Patch coverage is 65.60510% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 68.37%. Comparing base (f118ba2) to head (66f0495).
Report is 1 commits behind head on main.

Files	Patch %	Lines
go/vt/vttablet/tabletserver/schema/db.go	46.51%	23 Missing ⚠️
go/vt/vttablet/tabletserver/query_executor.go	12.50%	14 Missing ⚠️
go/vt/vtgate/schema/tracker.go	82.05%	7 Missing ⚠️
go/vt/vtgate/vschema_manager.go	83.78%	6 Missing ⚠️
go/vt/vtgate/vtgate.go	0.00%	2 Missing ⚠️
go/vt/vttablet/tabletserver/schema/engine.go	87.50%	1 Missing ⚠️
go/vt/vttablet/tabletserver/tabletserver.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15705      +/-   ##
==========================================
- Coverage   68.40%   68.37%   -0.03%     
==========================================
  Files        1556     1556              
  Lines      195121   195247     +126     
==========================================
+ Hits       133479   133508      +29     
- Misses      61642    61739      +97

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

deepthi · 2024-04-15T12:42:03Z

proto/query.proto

@@ -989,6 +992,7 @@ enum SchemaTableType {
  VIEWS = 0;
  TABLES = 1;
  ALL = 2;
+  UDF_AGGREGATE = 3;


Do we allow only aggregate UDFs? The vtgate flag is just called --enable_udfs. Release notes also say "any UDFs".

deepthi · 2024-04-15T12:54:43Z

go/vt/vttablet/endtoend/udfs_test.go

+// TestUDFRFC will validate that UDFs are received through the rfc call.
+func TestUDFRFC(t *testing.T) {


Is this supposed to read TestUDFRPC??

deepthi · 2024-04-15T12:56:12Z

proto/query.proto

@@ -883,6 +883,9 @@ message RealtimeStats {

  // view_schema_changed is to provide list of views that have schema changes detected by the tablet.
  repeated string view_schema_changed = 8;
+
+  // udfs_changed is used to signal that the UDFs have changed on the tablet.
+  bool udfs_changed = 9;


For tables and views, we track changes as list of changed objects. Why is this a single bool?
Can you have multiple .so files and/or multiple UDFs loaded? I assume so. In which case, if any of them changes, this will be true, and we will reload all of them?

For tables and views, the signal that a change has occurred is sent as the name of the table/view. Vtgate can then fetch the new definition for these objects when it wants that info. We did this so that the healtstream doesn't get weighed down with extra traffic that the vtgate might not be interested in - it can decide when to fetch the heavy data load.

For UDFs, the data load is the list and types of UDFs. The vtgate will simply reload the full list when something has changed. With this model, there is no need for the signal to specify what has changed.

deepthi · 2024-04-15T13:01:54Z

go/vt/vttablet/tabletserver/schema/db.go

+	detectUdfChange = `SELECT name
+FROM (
+	SELECT name FROM 
+	mysql.func 
+
+	UNION ALL
+
+	SELECT function_name
+	FROM %s.udfs
+) _inner
+GROUP BY name
+HAVING COUNT(*) = 1
+LIMIT 1


The way I'm reading this, we'll detect new or deleted UDFs, but not a change to the UDF definition?

Good point, we should also check the types

deepthi · 2024-04-15T13:02:40Z

go/vt/vttablet/tabletserver/schema/db.go

+
+	// copyUdfs copies user defined function to the udfs table.
+	copyUdfs = `INSERT INTO %s.udfs(FUNCTION_NAME, FUNCTION_RETURN_TYPE, FUNCTION_TYPE) 
+SELECT f.name, i.UDF_RETURN_TYPE, f.type FROM mysql.func f left join performance_schema.user_defined_functions i on f.name = i.udf_name


nit: I assume you use i as an alias for information_schema, so it makes sense to use p for performance_schema

deepthi · 2024-04-15T13:03:49Z

go/vt/vttablet/tabletserver/schema/db.go

+// GetFetchUDFsQuery gets the fetch query to retrieve all the UDFs.
+func GetFetchUDFsQuery(parser *sqlparser.Parser) (string, error) {


If we are fetching only aggregate UDFs, this func should be renamed to reflect that.

* cherry pick of 15705 * chore: boost integration Signed-off-by: Andres Taylor <andres@planetscale.com> --------- Signed-off-by: Andres Taylor <andres@planetscale.com> Co-authored-by: Andres Taylor <andres@planetscale.com>

GuptaManan100 and others added 3 commits April 11, 2024 13:24

feat: get if udfs have changed and send it as part of the broadcast

5fec02f

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add reloading for udfs data

5e1646a

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add UDF handling to the VSchemaManager

fad185c

Signed-off-by: Andres Taylor <andres@planetscale.com>

github-actions bot added this to the v20.0.0 milestone Apr 12, 2024

systay changed the title ~~feat: get if udfs have changed and send it as part of the broadcast~~ Track user defined aggregation functions for query planning Apr 12, 2024

harshit-gangal force-pushed the udf-schema-tracker branch from e86e1bd to 2706ae8 Compare April 12, 2024 07:26

systay changed the title ~~Track user defined aggregation functions for query planning~~ Add schema tracking support for UDFs Apr 15, 2024

systay force-pushed the udf-schema-tracker branch from 6248dc0 to 3f0ec63 Compare April 15, 2024 05:43

harshit-gangal force-pushed the udf-schema-tracker branch from 4ee6a93 to 80636f8 Compare April 15, 2024 07:51

harshit-gangal marked this pull request as ready for review April 15, 2024 08:20

harshit-gangal requested review from deepthi, harshit-gangal, mattlord, rohit-nayak-ps, derekperkins, shlomi-noach, frouioui, GuptaManan100 and arthurschreiber as code owners April 15, 2024 08:20

harshit-gangal added the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Apr 15, 2024

harshit-gangal added NeedsIssue A linked issue is missing for this Pull Request and removed NeedsWebsiteDocsUpdate What it says labels Apr 15, 2024

systay and others added 10 commits April 15, 2024 11:28

feat: add UDF logic to tracker and schema manager

568e082

Signed-off-by: Andres Taylor <andres@planetscale.com>

track all udf types and return udf name and udf return type for Aggre…

448a92f

…gate functions Signed-off-by: Harshit Gangal <harshit@planetscale.com>

check tablet health stats for udf changed flag and reload udfs, added…

0455103

… vtgate enableUDFs flag to keep it disabled by default Signed-off-by: Harshit Gangal <harshit@planetscale.com>

changed tracking queries and added e2e test on vttablet

0d76421

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

send udfs changed signal in health streamer, updated unit and e2e test

60f4c9f

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: added e2e test for schema rpc for udfs

7cacba5

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: skip e2e test on permission denied error

9cf4a89

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: updated helper output

593e814

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: updated expected sidecardb tables

952e1ea

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

release notes added

66f0495

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

systay force-pushed the udf-schema-tracker branch from 559b642 to 66f0495 Compare April 15, 2024 09:29

GuptaManan100 approved these changes Apr 15, 2024

View reviewed changes

go/vt/vtgate/vindexes/vschema.go Outdated Show resolved Hide resolved

harshit-gangal approved these changes Apr 15, 2024

View reviewed changes

harshit-gangal added Type: Bug Type: Regression and removed NeedsIssue A linked issue is missing for this Pull Request Type: Bug Type: Regression labels Apr 15, 2024

harshit-gangal merged commit 6b25965 into vitessio:main Apr 15, 2024
108 of 109 checks passed

harshit-gangal deleted the udf-schema-tracker branch April 15, 2024 12:20

deepthi mentioned this pull request Apr 15, 2024

Document UDF support vitessio/website#1726

Merged

deepthi reviewed Apr 15, 2024

View reviewed changes

This was referenced Apr 15, 2024

Gen4 Planner: support aggregate UDFs #15710

Merged

Prepare schema tracking for all UDFs #15732

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add schema tracking support for UDFs #15705

Add schema tracking support for UDFs #15705

systay commented Apr 12, 2024 •

edited by harshit-gangal

Loading

vitess-bot bot commented Apr 12, 2024 •

edited by deepthi

Loading

harshit-gangal commented Apr 15, 2024

GuptaManan100 left a comment

codecov bot commented Apr 15, 2024

deepthi Apr 15, 2024

deepthi Apr 15, 2024

deepthi Apr 15, 2024

systay Apr 16, 2024

deepthi Apr 15, 2024

systay Apr 16, 2024

deepthi Apr 15, 2024

deepthi Apr 15, 2024

		// TestUDFRFC will validate that UDFs are received through the rfc call.
		func TestUDFRFC(t *testing.T) {

		// GetFetchUDFsQuery gets the fetch query to retrieve all the UDFs.
		func GetFetchUDFsQuery(parser *sqlparser.Parser) (string, error) {

Add schema tracking support for UDFs #15705

Add schema tracking support for UDFs #15705

Conversation

systay commented Apr 12, 2024 • edited by harshit-gangal Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented Apr 12, 2024 • edited by deepthi Loading

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

harshit-gangal commented Apr 15, 2024

GuptaManan100 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 15, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

systay commented Apr 12, 2024 •

edited by harshit-gangal

Loading

vitess-bot bot commented Apr 12, 2024 •

edited by deepthi

Loading