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

Deprecate dbListResults() #58

Closed
krlmlr opened this issue Nov 10, 2015 · 9 comments
Closed

Deprecate dbListResults() #58

krlmlr opened this issue Nov 10, 2015 · 9 comments

Comments

@krlmlr
Copy link
Member

krlmlr commented Nov 10, 2015

Seems to be unsupported by existing backends, sometimes only one active result set is supported. Deprecate?

@hadley
Copy link
Member

hadley commented Nov 10, 2015

Yes, should be deprecated.

@krlmlr krlmlr changed the title Think about dbListResults() Deprecate dbListResults() Nov 11, 2015
@krlmlr krlmlr modified the milestone: 0.5 Jul 6, 2016
@bborgesr
Copy link
Contributor

bborgesr commented Jul 7, 2016

How to go about closing all open result sets if dbListResults is deprecated? This is important for pool when passivating a connection:

setMethod("onPassivate", "DBIConnection", function(object) {
  rs <- dbListResults(object)
  lapply(rs, function(x) {
    if (dbIsValid(x)) {
      dbClearResult(x)
      dbRollback(object)
    }
  })
})

@hadley
Copy link
Member

hadley commented Jul 7, 2016

You need to maintain your own list if that's important - there's no guarantee that that code would work correctly today.

@bborgesr
Copy link
Contributor

bborgesr commented Jul 7, 2016

@jcheng5 -- should we change this now? Or are we happy with using this method for the first release? It seems to work fine for RSQLite and MySQL, at least.

@jcheng5
Copy link

jcheng5 commented Jul 8, 2016

@hadley It's hard for us to maintain a list because we hand out connections with poolCheckout(), we're not privy to what happens to the connection after that.

What specifically about Barbara's code won't work correctly today? We know there's the problem with SQLite but dbIsValid seems to work around that. Will there be resultsets that are active but not returned?

@hadley
Copy link
Member

hadley commented Jul 8, 2016

@jcheng5 I don't think in general you should assume that dbListResults() is reliable - I don't remember the details, but I'm reasonably certain at least one of the big 3 has problems.

@bborgesr
Copy link
Contributor

Just for future reference: the deprecation of dbListResults() will no longer be an issue for pool after DBI implements a dbResetConnection() generic (see #117).

@krlmlr
Copy link
Member Author

krlmlr commented Jan 30, 2017

DBItest will test that an open result set is closed (with a warning) when the connection is closed. Also, I'm fine with restricting the number of simultaneously open result sets to 1 for now; implementing dbListResults() seems trivial in this scenario, so that it doesn't need to be deprecated. Would that help?

@bborgesr
Copy link
Contributor

@krlmlr: As far as pool is concerned, this should not be a problem for us anymore (it isn't now and I doubt it will be again in the future). We basically decided to stop trying to clean up after users, since DB drivers vary so much in their implementation (or lack thereof) of the same methods. So we're actually not using dbListResults() in pool anymore... Anything to add, @jcheng5?

@krlmlr krlmlr closed this as completed in 084c7ea Jan 27, 2018
krlmlr added a commit that referenced this issue Jan 27, 2018
- The `SQL()` function gains a `names` argument which can be used to assign names to SQL strings.
- `dbListResults()` is deprecated by documentation (#58).
- `dbGetException()` is soft-deprecated by documentation (#51).
- Help pages for generics now contain a dynamic list of methods implemented by DBI backends (#162).
- `sqlInterpolate()` now supports both named and positional variables (#216, @hannesmuehleisen).
- Specialized methods for `dbQuoteString()` and `dbQuoteIdentifier()` are available again, for compatibility with clients that use `getMethod()` to access them (#218).
krlmlr added a commit that referenced this issue Mar 9, 2018
…ps the names from the output if the `names` argument is unset. - The `dbReadTable()`, `dbWriteTable()`, `dbExistsTable()`, `dbRemoveTable()`, and `dbListFields()` generics now specialize over the first two arguments to support implementations with the `Id` S4 class as type for the second argument. Some packages may need to update their documentation to satisfy R CMD check again. New generics ------------ - Schema support: Export `Id()`, new generics `dbListObjects()` and `dbUnquoteIdentifier()`, methods for `Id` that call `dbQuoteIdentifier()` and then forward (#220). - New `dbQuoteLiteral()` generic. The default implementation uses switchpatch to avoid dispatch ambiguities, and forwards to `dbQuoteString()` for character vectors. Backends may override methods that also dispatch on the second argument, but in this case also an override for the `"SQL"` class is necessary (#172). Default implementations ----------------------- - Default implementations of `dbQuoteIdentifier()` and `dbQuoteLiteral()` preserve names, default implementation of `dbQuoteString()` strips names (#173). - Specialized methods for `dbQuoteString()` and `dbQuoteIdentifier()` are available again, for compatibility with clients that use `getMethod()` to access them (#218). - Add default implementation of `dbListFields()`. - The default implementation of `dbReadTable()` now has `row.names = FALSE` as default and also supports `row.names = NULL` (#186). API changes ----------- - The `SQL()` function gains an optional `names` argument which can be used to assign names to SQL strings. Deprecated generics ------------------- - `dbListConnections()` is soft-deprecated by documentation. - `dbListResults()` is deprecated by documentation (#58). - `dbGetException()` is soft-deprecated by documentation (#51). - The deprecated `print.list.pairs()` has been removed. Bug fixes --------- - Fix `dbDataType()` for `AsIs` object (#198, @yutannihilation). - Fix `dbQuoteString()` and `dbQuoteIdentifier()` to ignore invalid UTF-8 strings (r-dbi/DBItest#156). Documentation ------------- - Help pages for generics now contain a dynamic list of methods implemented by DBI backends (#162). - `sqlInterpolate()` now supports both named and positional variables (#216, @hannesmuehleisen). - Point to db.rstudio.com (@wibeasley, #209). - Reflect new 'r-dbi' organization in `DESCRIPTION` (@wibeasley, #207). Internal -------- - Using switchpatch on the second argument for default implementations of `dbQuoteString()` and `dbQuoteIdentifier()`.
@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants