Fix jdbc_fetch_size usage with postgresql (#103) #200
Conversation
|
Hi @izhigalko, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile? |
Thanks. I added and verified second email to my profile. |
| @@ -205,26 +207,29 @@ def execute_statement(statement, parameters) | |||
| query = @database[statement, parameters] | |||
| sql_last_value = @use_column_value ? @sql_last_value : Time.now.utc | |||
| @tracking_column_warning_sent = false | |||
| @logger.debug? and @logger.debug("Executing JDBC query", :statement => statement, :parameters => parameters, :count => query.count) | |||
There was a problem hiding this comment.
Why was :count removed? Did it execute an extra COUNT or was there some other reason?
There was a problem hiding this comment.
Count removed cause it slow down huge queries execution when debug enabled. My query have more than 5 subqueries in fields and i wait count execution about 5 hours before remove it.
lib/logstash/plugin_mixins/jdbc.rb
Outdated
| if @jdbc_paging_enabled | ||
| query.each_page(@jdbc_page_size) do |paged_dataset| | ||
| paged_dataset.each do |row| | ||
| @database.transaction do |
There was a problem hiding this comment.
Thanks for describing this change in the PR. We really need this documented here as well.
Do you know how this will play with other databases? I'm not anticipating problems ATM, but I'm curious what your thoughts are. I'm wondering if this codepath should be PG only.
There was a problem hiding this comment.
I think its ok with other dbs cause used standard JDBC transaction mechanic with Sequel::JDBC::Transactions, and all dbs that i knew uses transaction mechanic. I can rewrite code for use only with PG driver if u wish.
There was a problem hiding this comment.
@izhigalko no, I think that makes sense. I do think it needs a comment to explain why we wrap with a transaction then rollback for seemingly no reason however. If you put that comment in we can move forward :)
|
@izhigalko if you can just add in that comment I think we can move forward :) |
There was a problem hiding this comment.
@izhigalko it looks like the build is failing: https://travis-ci.org/logstash-plugins/logstash-input-jdbc/builds/228698596?utm_source=github_status&utm_medium=notification Can you take a look and fix? We'll need a passing build to merge :)
|
I'm very interested in seeing this move forward. I cloned locally to see if I could help fix the failing test, but when I ran them they all passed. Perhaps there was an issue with the Any chance we could run the build on this branch again to see if the tests pass? I'm suspicious that they will... |
|
@andrewvc ok, fix this. Test failed cause it uses mock database driver which not implements all needed methods |
|
Possible to have this revisited? The jdbc_fetch_size still doesn't work for postgresql. |
|
Hi all is jdbc_fetch_size working with postgresql now? I just using it in a project after switching from redshift and am running into memory errors. It worked great in redshift. |
According to documentation (https://jdbc.postgresql.org/documentation/head/query.html) we need to autocommit off if setFetchSize is used. So i wrap statement execution in transaction with Sequel::JDBC::Transaction cls and rollback transaction after all.