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

src_postgres needs to be deprecated everywhere #2688

Closed
istfer opened this issue Aug 26, 2020 · 6 comments · Fixed by #2881
Closed

src_postgres needs to be deprecated everywhere #2688

istfer opened this issue Aug 26, 2020 · 6 comments · Fixed by #2881
Assignees

Comments

@istfer
Copy link
Contributor

istfer commented Aug 26, 2020

Bug Description

src_postgres usage is deprecated but looks like it was not grepped and replaced everywhere. It still lives in the codebase in some places, e.g. 1, 2, ...

This is probably fine for majority of pecan users for now. But, for example, I use another port for the DB connection and these codes will not like that.

It is probably good to be consistent in how we open DB connection throughout the codebase anyway.

This is an issue to remind us to replace the remaining ones as well.

@istfer istfer self-assigned this Aug 26, 2020
@ashiklom
Copy link
Member

@moki1202 This might be another good quick issue for you to tackle. Briefly, we want to replace calls like this:

  bety <- dplyr::src_postgres(dbname   = dbparms$bety$dbname,
                              host     = dbparms$bety$host,
                              user     = dbparms$bety$user,
                              password = dbparms$bety$password)

...with the more general DBI calls like this:

  bety <- DBI::dbConnect(
    RPostgres::Postgres(),
    dbname   = dbparms$bety$dbname,
    host     = dbparms$bety$host,
    user     = dbparms$bety$user,
    password = dbparms$bety$password
  )
  con <- bety  # This alias may be necessary to work with our legacy code

@moki1202
Copy link
Collaborator

moki1202 commented Mar 2, 2021

@ashiklom ill take this issue into work after 3-4 days from now ...college exams going on rn.

@moki1202
Copy link
Collaborator

moki1202 commented Mar 7, 2021

@ashiklom while solving the above issue I found out that the file base\db\vignettes\betydb_access.Rmd still uses the library(dplyr) function . should I fix that ?

@moki1202
Copy link
Collaborator

moki1202 commented Mar 7, 2021

@ashiklom also the bottom line " con <- bety "...should this be " con <-bety$con " ?

@ashiklom
Copy link
Member

ashiklom commented Mar 8, 2021

I found out that the file base\db\vignettes\betydb_access.Rmd still uses the library(dplyr) function . should I fix that ?

No -- library calls are fine in R scripts and RMarkdown files (in fact, they are useful as documentation of which packages a script depends on). So I would leave it in.

also the bottom line " con <- bety "...should this be " con <-bety$con " ?

No. The old function, dbplyr::src_postgres (and similar), would return a list-like object containing the database connection itself (hence the ...$con) and some other dbplyr-specific metadata. However, the DBI method returns the connection directly. The con <- bety is just a hacky way to keep functions that use both bety and con objects still work. The more correct resolution is just to stick to one or the other. A quick grep of our codebase shows that con is the most popular occurrence, so probably best to use that one (my personal favorite -- dbcon is a distant third).

@istfer
Copy link
Contributor Author

istfer commented Aug 13, 2021

I'm late to the conversation, sorry about that, but please note that unless port is also passed to the DBI::dbConnect, connections won't open on our server where we have a different port

> dbcon <-DBI::dbConnect(
+   RPostgres::Postgres(),
+   host = 'localhost',
+   user = 'bety',
+   password = 'bety',
+   dbname = 'bety'
+ )
Error: FATAL:  Ident authentication failed for user "bety"
> dbcon
Error: object 'dbcon' not found

> dbcon <-DBI::dbConnect(
+   RPostgres::Postgres(),
+   host = 'localhost',
+   user = 'bety',
+   password = 'bety',
+   dbname = 'bety',
+   port = settings$database$bety$port
+ )
> dbcon
<PqConnection> bety@localhost:7654

Even when settings$database$bety$port is NULL, I think function would connect without errors using the default 5432. Please consider PR #2785 changes accordingly

@infotroph infotroph mentioned this issue Nov 1, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants