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 usage with postgresql (#103) #200

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions lib/logstash/plugin_mixins/jdbc.rb
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ def prepare_jdbc_connection
require "java"
require "sequel"
require "sequel/adapters/jdbc"
require "sequel/adapters/jdbc/transactions"
load_drivers(@jdbc_driver_library.split(",")) if @jdbc_driver_library

begin
Expand All @@ -157,6 +158,7 @@ def prepare_jdbc_connection
raise LogStash::ConfigurationError, "#{e}. #{message}"
end
@database = jdbc_connect()
@database.extend(Sequel::JDBC::Transactions)
@database.extension(:pagination)
if @jdbc_default_timezone
@database.extension(:named_timezones)
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was :count removed? Did it execute an extra COUNT or was there some other reason?

Copy link
Author

Choose a reason for hiding this comment

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

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.

@logger.debug? and @logger.debug("Executing JDBC query", :statement => statement, :parameters => parameters)

if @jdbc_paging_enabled
query.each_page(@jdbc_page_size) do |paged_dataset|
paged_dataset.each do |row|
@database.transaction do
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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 :)

if @jdbc_paging_enabled
query.each_page(@jdbc_page_size) do |paged_dataset|
paged_dataset.each do |row|
sql_last_value = get_column_value(row) if @use_column_value
if @tracking_column_type=="timestamp" and @use_column_value and sql_last_value.is_a?(DateTime)
sql_last_value=Time.parse(sql_last_value.to_s) # Coerce the timestamp to a `Time`
end
yield extract_values_from(row)
end
end
else
query.each do |row|
sql_last_value = get_column_value(row) if @use_column_value
if @tracking_column_type=="timestamp" and @use_column_value and sql_last_value.is_a?(DateTime)
sql_last_value=Time.parse(sql_last_value.to_s) # Coerce the timestamp to a `Time`
end
yield extract_values_from(row)
end
end
else
query.each do |row|
sql_last_value = get_column_value(row) if @use_column_value
if @tracking_column_type=="timestamp" and @use_column_value and sql_last_value.is_a?(DateTime)
sql_last_value=Time.parse(sql_last_value.to_s) # Coerce the timestamp to a `Time`
end
yield extract_values_from(row)
end
raise Sequel::Rollback
end
success = true
rescue Sequel::DatabaseConnectionError, Sequel::DatabaseError => e
Expand Down
2 changes: 1 addition & 1 deletion spec/inputs/jdbc_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,7 @@
end

it "should report the statements to logging" do
expect(plugin.logger).to receive(:debug).once
expect(plugin.logger).to receive(:debug).thrice
plugin.run(queue)
end
end
Expand Down