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

fix(column): avoid monkeypatching pg #271

Closed

Conversation

BuonOmo
Copy link
Collaborator

@BuonOmo BuonOmo commented Jun 23, 2023

Fixes #251

TODO:

  • verify that it works
  • implement tests

Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes seem fine to me. let's add a test to make sure the reported issue is resolved, and we don't regress.

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Jul 1, 2023

@rafiss I've been looking at how we could test this. And it seems like we would need an active PG connection. I'm not sure it is worth it having a PG connection for a non-regression on this. I have already spent 30m on trying to find a simpler way to test this. I think I could manage a hack but I would need much more time, and I am not sure it is worth it.

I would still do some more manual testing before merging though

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Jul 14, 2023

@rafiss so looking further, we also monkeypatched the method adapter_name_from. I think this PR could be changed to support multiple databases connections. WDYT?

@rafiss
Copy link
Contributor

rafiss commented Jul 20, 2023

I wanted to make sure I understand: Can you explain more about why making that change would support multiple database connections? And also could you define what you mean by "multiple database connections"?

@BuonOmo
Copy link
Collaborator Author

BuonOmo commented Jul 20, 2023

Can you explain more about why making that change would support multiple database connections?

Not sure which change you are talking about. But basically I want to make all of the sufficient changes to support multiple database connections. This may include more than the change made in this PR and the change mentioned in my above comment. For instance, I should un-exclude some tests and see what happen with those.

And also could you define what you mean by "multiple database connections"?

Quoting https://guides.rubyonrails.org/active_record_multiple_databases.html:

Rails now has support for multiple databases so you don't have to store your data all in one place.

At this time the following features are supported:

  • Multiple writer databases and a replica for each
  • Automatic connection switching for the model you're working with
  • Automatic swapping between the writer and replica depending on the HTTP verb and recent writes
  • Rails tasks for creating, dropping, migrating, and interacting with the multiple databases

These database can even be set with various adapters. This is what the issue #251 is about!


Back on what I said previously:

I'm not sure it is worth it having a PG connection for a non-regression on this.

We could do some testing by shipping a sqlite connection, this is way lighter and could work. But it is still a great amount of work, and I don't know that you'd want to invest that much on those kind of tests having the main tests already flaky. tl;dr: for me testing this further is a nice add.

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 this pull request may close these issues.

Incorrect db/schema.rb file for Postgres when using Cockroach and Postgres together.
2 participants