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

Fix jdbc_fetch_size with postgresql #29

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

Conversation

bmax
Copy link
Contributor

@bmax bmax commented Apr 25, 2020

Port from old library to new: logstash-plugins/logstash-input-jdbc#368

According to documentation (https://jdbc.postgresql.org/documentation/head/query.html) we need autocommit disabled. Work around is to create transaction and rollback always.

@bmax bmax force-pushed the fix_postgressql_fetching branch from 1798e97 to 39cfce3 Compare April 25, 2020 05:50
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @bmax - I have a couple of questions

@@ -1076,7 +1075,7 @@
end

it "should report the statements to logging" do
expect(plugin.logger).to receive(:debug).once
expect(plugin.logger).to receive(:debug).thrice
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know why this needed to change?

paged_dataset.each do |row|
# Execute query in transaction cause PG driver require autocommit off for set fetch count
# See: https://jdbc.postgresql.org/documentation/head/query.html
db.transaction(rollback: :always) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it might be possible to potentially use paged_each here?

If not, do we need to use transactions when paging is not enabled?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants