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

feat: all statement batching #108

Merged
merged 29 commits into from
Apr 18, 2022
Merged

feat: all statement batching #108

merged 29 commits into from
Apr 18, 2022

Conversation

libingye816
Copy link
Contributor

@libingye816 libingye816 commented Apr 8, 2022

Support batching of all statement types. Response messages are sent for each statement in the batch.
Cover most of the cases (not only the short-term solution to python driver) with the exception:

  1. The remaining statements in a dml/ddl batch are skipped and throw exceptions if the dml/ddl batch fails in the middle. The preceding statements in dml/ddl batch may not take effect even though they succeed. The whole batch resumes after the dml/ddl batch.

@libingye816 libingye816 changed the title Pgadapter Batching feat: all statement batching Apr 8, 2022
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! I've temporarily stopped reviewing at line 730 of JdbcSimpleModeMockServerTest.java, but I'll try to continue reviewing later today to give you a full review of everything.

Sorry for the confusion regarding error handling of implicit transactions. It seems that I got confused by the current implementation of batching when writing the first part of the design doc. The rule for what to do in case of an error in a batch is actually a lot simpler than I thought:

  1. Always continue executing statements in the batch.
  2. In case of implicit transaction: Execute each statement in the batch using auto-commit. This means that each statement that succeeds should be committed, regardless of whether other statements fail.

@libingye816
Copy link
Contributor Author

@olavloite Do you have an idea why clirr and e2e-psql-xxx are failing?

@olavloite
Copy link
Collaborator

olavloite commented Apr 13, 2022

The e2e psql tests are failing because some of those tests check whether they get an expected error when executing a mixed batch. That is no longer the case, as this PR introduces support for that. The psql tests work with text files that contain the expected output. If you take a look at the original draft PR, you will see that I changed the expected output of some of those (see for example dml-ddl-batching.txt: https://github.com/GoogleCloudPlatform/pgadapter/pull/30/files)

Clirr is failing because this PR introduces breaking changes in the public API of PGAdapter. I think we should disable clirr for this repository. It does not really make very much sense, as this is not a library that is imported as a dependency into other applications.

EDIT: I've removed the clirr check as a required check for this repo. So it is still there and will still fail for this PR, but it won't block merging this PR.

@libingye816
Copy link
Contributor Author

The e2e psql tests are failing because some of those tests check whether they get an expected error when executing a mixed batch. That is no longer the case, as this PR introduces support for that. The psql tests work with text files that contain the expected output. If you take a look at the original draft PR, you will see that I changed the expected output of some of those (see for example dml-ddl-batching.txt: https://github.com/GoogleCloudPlatform/pgadapter/pull/30/files)

I have already updated those expected output files. All tests were passed at last Friday, so I am surprised that they fail again. The psql receives error while expecting some outputs. Looks like the batch support doesn't take effect.

@libingye816 libingye816 requested a review from olavloite April 13, 2022 21:38
@libingye816
Copy link
Contributor Author

I fixed the logic of statement split to ignore escaped quotes. It should work now. @olavloite

Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me. I have some TODOs / nits in my comments, but feel free to defer those to a follow-up PR.

@libingye816
Copy link
Contributor Author

libingye816 commented Apr 16, 2022

Hi @olavloite Can you give a last round of quick review?

@libingye816 libingye816 merged commit 1d88311 into postgresql-dialect Apr 18, 2022
@libingye816 libingye816 deleted the batch-psycopg2 branch April 18, 2022 21:11
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