dbWithTransaction #110

bborgesr · 2016-06-06T03:38:56Z

Moving the relevant code for the dbWithTransaction() function to this PR, originally in #108

Note: The Travis Build is failing because the Examples section of dbWithTransaction() makes use of dbExecute(), which does not exist on this branch. So, this should not be a problem once we merge everything into master.

hadley · 2016-06-06T14:20:09Z

R/transactions.R

+  error = function(e) {
+    success <- dbRollback(conn)
+    if (!success) stop("`dbRollback` was not successful")


I think this error message needs to include the error message from e as well

Aren't those different things though? Independently of whether or not eoccurs, dbRollback should always succeed when called... And there might be a situation in which the user wants the transaction to fail, so he's expecting to see e, but not this other error related to dbRollback. By making those two things completely separate from one another, we don't risk any ambiguity. What do you think?

The problem is that currently if both the code and the rollback fail, you only see that the rollback fails. I don't think it's likely to happen in practice, but it seems like it's slightly safer to print the messages from all errors that occurred, not just the most recent error.

krlmlr · 2016-06-06T20:55:51Z

R/transactions.R

+setMethod("dbWithTransaction", "DBIConnection", function(conn, code) {
+  ## check if each operation is successful
+  if (!dbBegin(conn)) {


I see how dbBegin() etc. are currently declared, but I'd rather like the interface to raise an error if a transaction can't be opened, committed, or rolled back, #112. (An error during rollback is fatal in most cases, I'd expect a well-behaved DBMS to reject any further write operations in this case.)

But this is fine in the short-term, right? This works with the current interface that returns a logical value, and will also work with the future interface that throws errors. We can remove the redundant stop()s once most backends are updated to throw errors.

Oh, right, it works in both cases, but will break with DBI backends that don't return a logical. Perhaps something like if (isFALSE(dbBegin())) with a suitable implementation?

@krlmlr, Are there currently any backends that don't return a logical? From what I gather, it seems to me that you'd like to check if either dbBegin is FALSE or if it returns an error. If one of those conditions apply, then we should throw an error, right? If this is the case, it seems that the best check would be something like:

call <- dbBegin() if(!isTRUE(call) || inherits(call, "error")) stop(...)

It's slightly better to do

if (isFALSE(call)) stop(...)

because this prepares for "new-style" DBI backends that don't return a logical anymore. If dbBegin() throws an error, that code won't be reached anyway.

We'll have to define our own isFALSE().

Alright, I added the identical check. It doesn't really seem necessary to me to wrap it into a new isFALSE function though... The check is pretty straightforward and it doesn't take up that much space, so it feels like overkill (unless there's some weird performance improvement that I'm completely unaware of). Let me know if you feel strongly about this...
Otherwise, I think this is ready to be merged... ?

Thanks. The same logic should be applied to the dbCommit() and dbRollback() calls.

Could you please also merge the master branch into this branch and resolve the conflicts?

I'd be happy to, will you give me write access? Thanks!

We could also do it the other way round: You do

git fetch --all git merge origin/master # resolve conflicts git add ..... git commit

and then I can merge without conflicts.

@krlmlr, done :) Let me know if you encounter any issues.

@bborgesr

- Remove redundant declaration of transaction methods (#110, @@bborgesr).

@bborgesr

- Remove redundant declaration of transaction methods (#110, @@bborgesr). - New `dbExecute()`, forwards to `dbSendQuery()` by default (#109, @bborgesr).

Merge remote-tracking branch 'dbi/master' into dbWithTransaction # Conflicts: # NAMESPACE # NEWS.md # R/DBConnection.R # R/transactions.R # man/dbDisconnect.Rd # man/dbExecute.Rd # man/dbExistsTable.Rd # man/dbGetException.Rd # man/dbGetQuery.Rd # man/dbListFields.Rd # man/dbListResults.Rd # man/dbListTables.Rd # man/dbReadTable.Rd # man/dbRemoveTable.Rd # man/dbSendQuery.Rd

codecov-io · 2016-06-14T13:28:39Z

Current coverage is 43.80%

Merging #110 into master will decrease coverage by 2.43%

@@             master       #110   diff @@
==========================================
  Files            15         16     +1   
  Lines           413        436    +23   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            191        191          
- Misses          222        245    +23   
  Partials          0          0

Powered by Codecov. Last updated by 5213bf6...53105bc

krlmlr · 2016-06-14T14:19:34Z

NEWS.md

@@ -1,9 +1,3 @@
-# DBI 0.4-2 (2016-06-08)


I'd like to keep those NEWS.

oops, sorry! Re-added (and removed the extra @)

krlmlr · 2016-06-14T14:25:58Z

LGTM, apart from the NEWS removal.

krlmlr · 2016-06-14T14:29:42Z

Thanks!

@bborgesr

- The default implementation of `dbQuoteString()` doesn't call `encodeString()` anymore: Neither SQLite nor Postgres understand e.g. `\n` in a string literal, and all of SQLite, Postgres, and MySQL accept an embedded newline (#121). - New `dbWithTransaction()` that calls `dbBegin()` and `dbCommit()`, and `dbRollback()` on failure (#110, @bborgesr).

@bborgesr

- Interface changes - `dbDataType()` maps `character` values to `"TEXT"` by default (#102). - The default implementation of `dbQuoteString()` doesn't call `encodeString()` anymore: Neither SQLite nor Postgres understand e.g. `\n` in a string literal, and all of SQLite, Postgres, and MySQL accept an embedded newline (#121). - Interface enhancements - New `dbSendStatement()` generic, forwards to `dbSendQuery()` by default (#20, #132). - New `dbExecute()`, calls `dbSendStatement()` by default (#109, @bborgesr). - New `dbWithTransaction()` that calls `dbBegin()` and `dbCommit()`, and `dbRollback()` on failure (#110, @bborgesr). - New `dbBreak()` function which allows aborting from within `dbWithTransaction()` (#115, #133). - Export `dbFetch()` and `dbQuoteString()` methods. - Documentation improvements: - One example per function (except functions scheduled for deprecation) (#67). - Consistent layout and identifier naming. - Better documentation of generics by adding links to the class and related generics in the "See also" section under "Other DBI... generics" (#130). S4 documentation is directed to a hidden page to unclutter documentation index (#59). - Fix two minor vignette typos (#124, @mdsumner). - Add package documentation. - Remove misleading parts in `dbConnect()` documentation (#118). - Remove misleading link in `dbDataType()` documentation. - Remove full stop from documentation titles. - New help topic "DBIspec" that contains the full DBI specification (currently work in progress) (#129). - HTML documentation generated by `staticdocs` is now uploaded to http://rstats-db.github.io/DBI for each build of the "production" branch (#131). - Further minor changes and fixes. - Internal - Use `contains` argument instead of `representation()` to denote base classes (#93). - Remove redundant declaration of transaction methods (#110, @bborgesr).

bborgesr added 2 commits June 6, 2016 04:36

dbWithTransaction

ac29ca1

removed NEWS update

8d4fdd9

bborgesr mentioned this pull request Jun 6, 2016

dbGetChunkedQuery, dbExecQuery and withTransaction #108

Closed

hadley reviewed Jun 6, 2016
View reviewed changes

improved error messages

93d6c3d

krlmlr reviewed Jun 6, 2016
View reviewed changes

krlmlr pushed a commit that referenced this pull request Jun 8, 2016

remove dupe

2f60206

- Remove redundant declaration of transaction methods (#110, @@bborgesr).

krlmlr pushed a commit that referenced this pull request Jun 10, 2016

Merge tag 'v0.4-2'

5213bf6

- Remove redundant declaration of transaction methods (#110, @@bborgesr). - New `dbExecute()`, forwards to `dbSendQuery()` by default (#109, @bborgesr).

bborgesr added 3 commits June 13, 2016 14:42

included error in rollback fail

c8a8b64

better error handling

e4818de

krlmlr reviewed Jun 14, 2016
View reviewed changes

krlmlr added the in progress label Jun 14, 2016

re-added NEWS items

8724524

krlmlr merged commit a1367f7 into r-dbi:master Jun 14, 2016

krlmlr removed the in progress label Jun 14, 2016

krlmlr mentioned this pull request Jul 31, 2016

Add transaction helper #104

Closed

github-actions bot locked and limited conversation to collaborators Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dbWithTransaction #110

dbWithTransaction #110

bborgesr commented Jun 6, 2016 •

edited

Loading

hadley Jun 6, 2016

bborgesr Jun 6, 2016

hadley Jun 6, 2016

krlmlr Jun 6, 2016

hadley Jun 6, 2016

krlmlr Jun 7, 2016

bborgesr Jun 13, 2016

krlmlr Jun 13, 2016 •

edited

Loading

bborgesr Jun 13, 2016

krlmlr Jun 13, 2016

bborgesr Jun 13, 2016

krlmlr Jun 14, 2016

bborgesr Jun 14, 2016

codecov-io commented Jun 14, 2016 •

edited

Loading

krlmlr Jun 14, 2016

bborgesr Jun 14, 2016

krlmlr commented Jun 14, 2016

krlmlr commented Jun 14, 2016

dbWithTransaction #110

dbWithTransaction #110

Conversation

bborgesr commented Jun 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krlmlr Jun 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 14, 2016 • edited Loading

Current coverage is 43.80%

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krlmlr commented Jun 14, 2016

krlmlr commented Jun 14, 2016

bborgesr commented Jun 6, 2016 •

edited

Loading

krlmlr Jun 13, 2016 •

edited

Loading

codecov-io commented Jun 14, 2016 •

edited

Loading