-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Implement server side cursors #18047
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
base: main
Are you sure you want to change the base?
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
20d5d8f to
5b481d5
Compare
|
Re-pushed with updated commit description because I just noticed I misattributed some of the original work this change is based on (I copied the author from a wrong commit). Want to make sure I give credit where it's due. (: |
5b481d5 to
ba5ef51
Compare
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
850cfef to
bbf12b9
Compare
bbf12b9 to
58fffbd
Compare
harshit-gangal
left a comment
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.
You would need to add e2e test to validate it works cleanly with fdw
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
58fffbd to
55d0b81
Compare
|
There are some CI failures that needs to be addressed. |
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
55d0b81 to
5c80efd
Compare
I agree a E2E test for this would be useful to prevent this from breaking in the future. However, I'm not entirely sure how to go about implementing this E2E test within the template structure used for generating the workflows. I'd very much welcome if anyone else wants to help contribute such a test! The basic test would work like this:
Also note that while compatibility with PostgreSQL FDW happens to be my use case, there is nothing specific to PostgreSQL in the changes made in this PR. It implements support for cursors in the MySQL protocol, and thus should be useful to anyone that wants to use cursors for any reason. Presumably there could also be E2E tests that simply use a cursor from a regular MySQL client. I think having a specific E2E test for PostgreSQL FDW, while definitely a nice to have, doesn't have to be a specific blocker for this PR. |
This is exactly what I am looking for in our e2e test. we do not want postgres running in our CI |
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
5c80efd to
6d1bc0a
Compare
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
To be able to use the MySQL foreign data wrapper for Postgres, we have to support server side cursors. To that end each connection now keeps track of a list of open cursors, and implements the COM_STMT_FETCH command for retrieving data from an open cursor. Based on the implementation in the Dolthub fork of Vitess: dolthub#228 dolthub#232 Co-Authored-By: James Cor <[email protected]> Co-Authored-By: Aaron Son <[email protected]> Signed-off-by: Daniël van de Giessen <[email protected]>
The previous commit which implemented support for server side cursors also updated the function signature of the MySQL query functions to return a integer "status" value instead of a boolean "more" value. One of the flags in that status integer represents the "more" flag. This commit updates all places where the MySQL query functions are called to use this new status integer. Signed-off-by: Daniël van de Giessen <[email protected]>
6d1bc0a to
0c5f127
Compare
Description
To be able to use the MySQL foreign data wrapper for PostgreSQL, we have to support server side cursors.
To that end each connection now keeps track of a list of open cursors, and implements the COM_STMT_FETCH command for retrieving data from an open cursor.
Based on the implementation in the Dolthub fork of Vitess:
Compared to the implementation in the Dolthub fork this commit:
I've also moved all the changes to other code (namely, updating them to use the "status" integer instead of the old "more" boolean) into a separate commit to make it easier to review.
Related Issue(s)
#18046
Checklist
Deployment Notes
This only implements a new protocol feature and does not affect any on-disk information. No migrations should be required.