Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support optional resultset metadata #1150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tz70s
Copy link

@tz70s tz70s commented Aug 30, 2020

Allow optional resultset metadata.
Can potentially improve the performance in many scenario.

Issue #1105

Note

I believe this flag was introduced in MySQL 8.0+ only.

Some open questions are:

  • Should we introduce this flag/system var?
  • If yes, how can we let user know the version they operate on is compatible with this flag or not? I believe we had some existing usage that may break versions, some documentation added will be great.
  • I don't really feel like current tests against MySQL matrix on CI w/ different versions are reliable, maybe a better way is catching up environment variable (e.g. DB) to test each version, instead of relying on maybeSkip.

Need your feedback on this, thank you!

Description

Add support optional resultset metadata.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@tz70s tz70s force-pushed the support-resultset-optional-metadata branch 2 times, most recently from d9cf63e to 46e70d0 Compare August 30, 2020 02:11
@tz70s tz70s changed the title WIP: Support optional resultset metadata Support optional resultset metadata Aug 30, 2020
@tz70s tz70s force-pushed the support-resultset-optional-metadata branch 7 times, most recently from 21f614e to 38aa54a Compare August 30, 2020 05:22
@tz70s
Copy link
Author

tz70s commented Aug 30, 2020

Not really sure why seems this test is flaky in OSX travis

=== RUN   TestConcurrent
    driver_test.go:1859: testing up to 151 concurrent connections 
    driver_test.go:1913: reached 151 concurrent connections
    driver_test.go:1859: testing up to 151 concurrent connections 
    driver_test.go:1913: reached 150 concurrent connections
    driver_test.go:1859: testing up to 151 concurrent connections 
    driver_test.go:1910: error on conn 12: dial tcp 127.0.0.1:3306: connect: connection reset by peer
--- FAIL: TestConcurrent (1.09s)

@tz70s tz70s force-pushed the support-resultset-optional-metadata branch from 51e4ca4 to b819a69 Compare August 30, 2020 23:36
README.md Outdated

Allow resultset metadata being optional.
By making resultset metadata transfer being optional, can potentially improve queries performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Document that this requires MySQL 8.0+.

Add link to MySQL documentation: https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_resultset_metadata

dsn.go Outdated
@@ -465,6 +470,18 @@ func parseDSNParams(cfg *Config, params string) (err error) {
return errors.New("invalid bool value: " + value)
}

// allow resultset metadata being optional
case "resultSetMetadata":
Copy link
Contributor

Choose a reason for hiding this comment

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

As a mysql variable resultset_metadata already exists, it would be better to use it (like time_zone).

connector.go Outdated
@@ -129,6 +129,24 @@ func (c *connector) Connect(ctx context.Context) (driver.Conn, error) {
mc.maxWriteSize = mc.maxAllowedPacket
}

// Additional handling for result set optional metadata
if mc.cfg.ResultSetMetadata != "" {
err = mc.exec("SET resultset_metadata=" + mc.cfg.ResultSetMetadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

handleParams sends all SET at once. It would be better to use that. See also my remark about the option name.

dsn.go Outdated
Timeout time.Duration // Dial timeout
ReadTimeout time.Duration // I/O read timeout
WriteTimeout time.Duration // I/O write timeout
ResultSetMetadata string // Allow optional resultset metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed if handled il Params like other MySQL variables.

errors.go Outdated
ErrPktTooLarge = errors.New("packet for query is too large. Try adjusting the 'max_allowed_packet' variable on the server")
ErrBusyBuffer = errors.New("busy buffer")
ErrNoOptionalResultSet = errors.New("requested optional resultset metadata but server does not support")
ErrOptionalResultSetPkt = errors.New("malformed optional resultset metadata packets")
Copy link
Contributor

Choose a reason for hiding this comment

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

Those variable should contain Metadata in the name, because that's not the resultset which is optional but just the metadata.

@tz70s tz70s force-pushed the support-resultset-optional-metadata branch 3 times, most recently from cb6c794 to acbdeec Compare January 21, 2022 01:29
Allow optional resultset metadata.
Can potentially improve the performance in many scenarios.

Issue go-sql-driver#1105
@tz70s tz70s force-pushed the support-resultset-optional-metadata branch from acbdeec to 0b7ff91 Compare January 21, 2022 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants