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

Support PostgreSQL materialized views #13128

Closed
wants to merge 5 commits into from

Conversation

kastiglione
Copy link
Contributor

The change is pretty minimal, the query used in #table_exists? has been expanded to include materialized views in the kinds of relations it includes in its search.

In order to share the existing view tests, I made the previous test class into a concern that is then included into separate ViewTest and MaterializedViewTest classes, the latter only if PostgreSQL is at least version 9.3. The test class then uses the name of the test to determine the type of view to create and test against.

Expand the query used in #table_exists? to include materialized views in the
kinds of relations it searches.
@kastiglione
Copy link
Contributor Author

I've had a look at the Travis logs, but there isn't much to go off. Is there a way to find out info about whichever error was raised?

@@ -1,11 +1,26 @@
require "cases/helper"

module ViewTestConcern
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to reopen the same module in the same file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that purely for top-down readability. If I define ViewTestConcern entirely up front, that pushes the two test class definitions to the bottom of the file. I thought it would be preferable to put them at the top. So no technical reason, it can be changed.

@kastiglione
Copy link
Contributor Author

Note that in dd3aba0 I did a bit of clean-up by removing #extract_pg_identifier_from_name which duplicates the behavior of Utils.extract_schema_and_table. The former was used in just one method, and has no tests while the latter does have tests.

@kastiglione
Copy link
Contributor Author

😎

Apologies if it's bad form to bump a PR, I'm just unsure that this won't get lost in the crowd.

@senny
Copy link
Member

senny commented Dec 19, 2013

@kastiglione Will work on this once 4.1 is out. I assigned the 4.2 milestone.

@ghost ghost assigned senny Dec 19, 2013
@kastiglione
Copy link
Contributor Author

✨ thanks @senny

@senny
Copy link
Member

senny commented Feb 26, 2014

@kastiglione 4-1-stable is forked off and we can move this forward. After skimming through the changes I noticed there are quite a bit of unrelated changes. Can you cherry-pick the refactoring changes onto another branch and submit a PR from there? I'd like to keep this one focused on adding support for materialized views only.

The refactorings might have a different impact. For example it favors the slower extract_schema_and_table method. See this gist. We should evaluate these in a separate PR.

@kastiglione
Copy link
Contributor Author

Sure, sounds good. Was trying to clean up what I saw as obvious duplication, but indeed they are separate changes.

@senny
Copy link
Member

senny commented Mar 28, 2014

@kastiglione ping, wanted to check back if you have time to work on this? Please let me know if you don't so I can move this forward. Thank you for your work 💛

@kastiglione
Copy link
Contributor Author

I don't have much time, but also work impediments besides time. Can I simply revert the last two commits, the ones that mucked with schema extraction? The first three commits were the ones core to supporting materialized views.

Thanks to you as well.

@senny
Copy link
Member

senny commented Mar 28, 2014

yes, you can. Make sure to squash the other commits into a single one and rebase your branch. It does no longer apply.

@senny
Copy link
Member

senny commented Apr 2, 2014

I squashed the commits and applied the patch via cherry-pick (def6071)

@kastiglione thank you for your work!

@senny senny closed this Apr 2, 2014
@kastiglione
Copy link
Contributor Author

Thanks @senny. My apologies for any delay I might have caused.

@kastiglione kastiglione deleted the pg-materialized-views branch April 2, 2014 08:56
@senny
Copy link
Member

senny commented Apr 2, 2014

@kastiglione no worries, we have plenty of time until 4.2 hits 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants