Feature/query timeout option

[Odraio]

Issue

Current odbc main repo does not provide a query_timeout option.

Closes #515

Feature description

dbGetQuery(), dbSendStatement() and dbSendQuery() provide a query_timeout option (internally sets SQL_ATTR_QUERY_TIMEOUT).

query_timeout: The number in seconds before query timeout (can be used to stop long running queries).
Default is 0 indicating no timeout.

Example:

# long running query stops after 60 seconds
# provided statement is a placeholder

dbGetQuery(con, "SELECT flight, tailnum, origin FROM flights ORDER BY origin", query_timeout = 60)

dbSendStatement(con, "SELECT flight, tailnum, origin FROM flights ORDER BY origin", query_timeout = 60)

dbSendQuery(con, "SELECT flight, tailnum, origin FROM flights ORDER BY origin", query_timeout = 60)

Additional request:

• After merging, please create a new (CRAN) release.

[detule]

Hi - thanks for the submission.

It would be challenging to review this PR because of the way the diff comes through. You may be using text editing software (windows?) that alters the line endings. Please resubmit with a clean diff.

Also, commenting out the ASSERT is likely not the right approach. I would leave that line unaltered in your next submission; we can address elsewhere.

[Odraio]

@detule I've pushed a new version of the branch which results in a clean diff. In addition the workaround of #512 has been reverted.

[Odraio]

There is one issue while using the query_timeout and params. I will check and fix this!

Update: Issue has been detected, I will commit the fix tomorrow.

[Odraio]

A new fix has been pushed which resolves the issue of a query using params and query_timeout. Additional tests have been added as well. Pull request is ready for review.

[Odraio]

@detule Did you have an opportunity to review and approve the pull request?

[shrektan]

It looks good to me overall. Thanks for your contributions.

• Could you please add the doc for query_timeout as well?
• It would be great if you could also update the NEWS.md file.
• Lastly, how about using a shorter param name "timeout"?

Thanks.

[detule]

Hi:

Thanks for the submission and for adding tests - appreciate it. Mostly small comments:

• Second @shrektan 's suggestions:
  • Rename the parameter to timeout, and the NEWS entry.
  • Move the documentation out of the README to the roxygen doc blocks for all [R] methods that gained a timeout parameter.
• Not sure why the DB tests are failing, thought at some point I saw them passing on this branch. But we need to figure that out; as best as i can tell they are still passing on main.

[Odraio]

@detule and @shrektan Thanks for reviewing! I will update the pull request later on this week.

It seems there is always some confusion between a connection timeout and an actual query timeout. To avoid this, I've choosen for the query_timeout parameter name. Is it still required to rename query_timeout to timeout (shorter names aren't always better in my opinion...)

@detule

Move the documentation out of the README to the roxygen doc blocks for all [R] methods that gained a timeout parameter.

Why should it be moved out of the README? I think it should be available as well in the README section on the GitHub home page of the project. Users will immediatly have an example.
In addition, could you please provide a link where I can find the roxygen doc blocks?

[detule]

Hi:

• Parameter name: The use of the timeout argument to dbConnect is documented in the roxygen tags for that function; I think that should clear up ambiguity between dbConnect(..., timeout,... ) and dbGetQuery(..., timeout, ...). In addition, and admittedly this is a bit subjective, but dbGetQuery(..., query_timeout, ...) feels like repeated usage of "query".

• In terms of the README: I think that document covers some basic functionality of the odbc package - features that are (mostly) guaranteed to work across databases and various drivers; note there are many details missing, especially some that are not (guaranteed to be) universally supported. I think the timeout parameter to the query methods falls in that latter category. It is a very useful addition - I thought about maybe adding your examples in an @examples block, but I don't even think there is enough complexity in the usage of the new timeout parameter that would warrant that. Its purpose and usage is pretty clear-cut!

  Between the announcement in the NEWS file, and the added documentation, I think the feature will be well advertised and understood.

Thanks again for the submission.

[hadley]

@Odraio can you reproduce build failures locally? It works for my when I test with my local sql server, so I'm a bit mystified as to what is going wrong.