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

Add generic dbIsReadOnly #190

Merged
merged 1 commit into from
Mar 26, 2018
Merged

Conversation

anhqle
Copy link

@anhqle anhqle commented Jul 24, 2017

From Issue #189

Users that use a read-only mode would likely want a way to periodically check whether their connection is truly read only.

For my own backend, I've added this generic, but want to suggest this feature here. I can contribute a PR if there is interest.

Closes #189

@codecov-io
Copy link

codecov-io commented Jul 24, 2017

Codecov Report

Merging #190 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #190   +/-   ##
=======================================
  Coverage   43.34%   43.34%           
=======================================
  Files          16       16           
  Lines         466      466           
=======================================
  Hits          202      202           
  Misses        264      264
Impacted Files Coverage Δ
R/DBObject.R 0% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 134eddc...e550db6. Read the comment docs.

@krlmlr
Copy link
Member

krlmlr commented Nov 27, 2017

Thanks. I wonder in which situations the read-only status of an existing connection can change?

@anhqle
Copy link
Author

anhqle commented Nov 28, 2017

I agree that it's unlikely for a read-only connection to change its read-only status.

The generic is more of an UX improvement, e.g. confirming the read-only status whenever the user print(connection).

Given that an user may initiate a connection and work with it over hours, such a reminder is re-assuring.

(Tagging @wlattner, in case this DBI feature is something civis-r still uses).

@krlmlr
Copy link
Member

krlmlr commented Dec 18, 2017

Thanks, I'm happy to include it. How do you open a read-only connection to a database? Does this method also cover situations where the read-only status is due to missing write permissions?

@anhqle
Copy link
Author

anhqle commented Dec 19, 2017

An user opens a read-only connection with CivisConnection(read_only = TRUE, ...).

Then, whenever the user sends a query (via dbSendQuery) using this read-only connection, we prepend the query with BEGIN READ ONLY (as instructed by Amazon Redshift documentation, which we use).

Our R package doesn't deal with permission. It simply sends users' queries from R to their databases hosted on our cloud. We check permissions in the cloud.

@krlmlr
Copy link
Member

krlmlr commented Jan 25, 2018

I wonder if we should provide a default method for "DBIConnection"?

Could you please also update the documentation?

@krlmlr krlmlr merged commit 5e394a6 into r-dbi:master Mar 26, 2018
@krlmlr
Copy link
Member

krlmlr commented Mar 26, 2018

Thanks! Let's avoid a default method for now.

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants