-
Notifications
You must be signed in to change notification settings - Fork 21
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
Added support for MariaDb extend type info #288
Open
svats0001
wants to merge
15
commits into
asyncer-io:trunk
Choose a base branch
from
svats0001:trunk
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
ac7a5fe
Added support for MariaDb extend type info
svats0001 c11a2bf
Merge branch 'trunk' into trunk
svats0001 cc72c61
MariaDb extended type info changes
svats0001 af3c861
Altered DefinitionMetadataMessage
svats0001 14fedd3
Added enabledif to integration test
svats0001 518c1c5
Testing decode
svats0001 088f8f6
Testing decode
svats0001 567dc8d
Changed decode41
svats0001 4ff52dc
Changed integration test
svats0001 b05d64c
Fixed integration test
svats0001 6211e57
Testing capability
svats0001 a5750a9
Added 11.5.2 to ci-mariadb-integration
svats0001 c7bece4
Only testing 11.5.2
svats0001 1d91f3e
Changed decode41 and reset ci-mariadb-integration
svats0001 42da732
Reset decode41
svats0001 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I add the
MARIADB_CLIENT_EXTENDED_TYPE_INFO
capability toALL_SUPPORTED
, I get an error. Could you check this out?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I was testing it the MariaDB versions >= 10.5 didn't seem to return the extended_type_info capability which is what confused me as I manually enabled these capabilities in the test and it didn't work because the packet didn't contain the extended information. In this case the capability is enabled by default but the packet doesn't contain the extended information so if anything you shouldn't include the extended_type_info capability in ALL_SUPPORTED and instead rely on the server returning the capability if enabled. But then the capability should have been turned off if not enabled by the server should it not..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I understand. In my setup(and github), the capability variable represents the server-side capability, which is provided directly by the server. This capability includes extended_type_info, but the capability.extendMariaDb line here removes this flag due to the influence of ALL_SUPPORTED.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svats0001 Could you try using MariaDB version 11.5.2? Alternatively, you could run the test with the VM options
-Dtest.db.type=mariadb -Dtest.db.version=11.5.2
. The official Docker image supports the flag by default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I overlooked that fact about ALL_SUPPORTED, that makes sense.
I've tried with version 11.5.2 as well and it's not working either. The fact that all the tests run fine without trying to read extended_type_info suggests that the column definition packets don't contain this information. In the docs for extended_type_info, it does say 'while string has data' which may mean that it only returns the data for specific columns, so I'll play around with that some more by testing conditionally on the value of the first byte. Also, even versions < 10.5 return the extended_type_info flag because the tests fail for these versions too which is weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I'm not able to progress this issue any further, I'll close this pull request if that's okay @jchrys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svats0001
Would it be possible to keep this PR open so that I can continue working on it when I have some availability? I believe this work is valuable and deserves to be delivered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep no problem, I'll leave it open.