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 new module: materialized sql view #110

Closed
wants to merge 28 commits into from

Conversation

petrus-v
Copy link
Contributor

@petrus-v petrus-v commented Jan 9, 2015

Hi,

I'm Pierre Verkest from Anybox, I'm pretty new in OCA and not sure about
the workflow to add new modules. Anyway this is a module we developed
on bitbucket.

I'm glad to contribute and dive into OCA tools and thanks @legalsylvain who
ask us to push this materialized_sql_view module here.

If you want to know more about this module you can read materialized_sql_view
module description in __openerp__.py.

I had to separate tests in an other module as I would tested the module with a
real model but wouldn't see it on production servers.

I already prepare a branch running under v7
I guess if it's like odoo workflow I haven't to make a PR on both branches (7.0 and 8.0) ?

I was surprised some tested pass in Travis, like this one:
https://travis-ci.org/petrus-v/server-tools/builds/46459751 based on this
commit: petrus-v@ab614ed
which should failed.

regards,

@coveralls
Copy link

Coverage Status

Coverage increased (+11.1%) when pulling 53ae7ee on petrus-v:8.0-materialized_sql_view into 271e515 on OCA:8.0.

# =============================================================================
{
'name': 'Materialized Sql View',
'version': '0.0.1',

Choose a reason for hiding this comment

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

I think 0.1 would suffice.

@clonedagain
Copy link

That looks nice. I wonder if you couldn't simplify the code by dropping the legacy postgresql option. PG 9.3 has been available for quite some time and is the version recommanded by odoo SA.

@coveralls
Copy link

Coverage Status

Coverage increased (+11.1%) when pulling e916338 on petrus-v:8.0-materialized_sql_view into 271e515 on OCA:8.0.

@petrus-v
Copy link
Contributor Author

petrus-v commented Jan 9, 2015

@clonedagain,
Yes it could be easier if we had to support only one PG version, but we made it because we need it for our customer. Unfortunately we don't manage hosting for him, and it can take a while to make a plan with three parts (customer/hoster/IT integrator).

I could say with experience, companies (mainly true bigger they are) are afraid about change when we talk about their tool whose all employees depends so I'm sure I will find PG9.2 on servers for a couple years!

As you can see, if the PG cluster version is upgraded the module take care about it and will drop and recreate the view against the good version.

I hope the effort to maintain multi PG version will be benefit for the community we have also ideas to enhance this module which could explain some choices:

  • PG9.4 which was released in December introduce a new option CONCURRENTLY which could be easily introduce in this module by patterns we had chosen.
  • using different strategies to refresh materialized view
  • Display on the UI the last refresh date until data was collected
  • Provide a dependency refresh engine (in case of some materialized view depend of some other)

Thanks for your review, I already push the version to 0.1 as recommended.

@petrus-v
Copy link
Contributor Author

I had not seen that @legalsylvain made the issue #101 before asking us by email which is related to this PR.

'name': 'Materialized Sql View',
'version': '0.1',
'category': 'Tools',
'description': """
Copy link
Member

Choose a reason for hiding this comment

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

In v8.0 description can be put in README.rst to be visible both in github and on Odoo

You can find a sample here:
https://github.com/OCA/maintainers-tools/blob/master/template/module/README.rst

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for having created README.rst

Now you just need to remove description key from __openerp__.py

@yvaucher
Copy link
Member

fr.po is there but pot file is missing

@hbrunn
Copy link
Member

hbrunn commented Feb 2, 2015

@clonedagain current CentOS ships with postgresql 9.2, so I think it's definitely worthwhile to support that. The code looks good to me, after the README.rst issue and the missing pot file is fixed, I think this is good to merge.

@legalsylvain
Copy link
Contributor

👍 (Code review, No test.)

@petrus-v
Copy link
Contributor Author

petrus-v commented Feb 2, 2015

Sorry I'm quite busy actually, sure I will take in consideration your advices. I also have to push a fixe that we figure out using it on production (a use case when it's crash by space missing on the Postgresql temp directory partition ) !

I'll do it this week!

@petrus-v
Copy link
Contributor Author

petrus-v commented Feb 2, 2015

My apologies to reviewers I've just realized it's probably not fair to add capabilities (commit cbbdc16) after your reviews on the module.

@clonedagain
Copy link

@petrus-v I think it's OK to improve before merging, as long as you notify the reviewers. I prefer that than a new PR.

@yvaucher
Copy link
Member

yvaucher commented Feb 3, 2015

Restarted 3rd job of travis. Due to an error when pulling dependencies:

ImportError: No module named pytz

Not the first time I see this.

@yvaucher
Copy link
Member

yvaucher commented Feb 3, 2015

BTW @petrus-v don't mind squashing those pep8 / pylint commit. That doesn't help much in history.

@petrus-v petrus-v force-pushed the 8.0-materialized_sql_view branch from 7e2e4e9 to 00f32e5 Compare February 17, 2015 11:01
@petrus-v
Copy link
Contributor Author

I've just rebase the PR to have a look what's happen!

@max3903 max3903 removed the 8.0 label Feb 17, 2015
@max3903 max3903 added this to the 8.0 milestone Feb 17, 2015
@petrus-v petrus-v force-pushed the 8.0-materialized_sql_view branch from 00f32e5 to 2b59a21 Compare June 2, 2015 13:39
@StefanRijnhart
Copy link
Member

Looks good (no test). Is travis just not completing its job or has it just been restarted?

'name': 'Materialized Sql View',
'version': '0.1',
'category': 'Tools',
'author': 'Pierre Verkest',
Copy link

Choose a reason for hiding this comment

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

Please put OCA in the author as second author

Pierre Verkest added 19 commits October 4, 2016 16:08
when updating modules containing materialized sql view, _sql_mat_view_name
wasn't defined, so the sql view was always recreated on updating
In some case it could result in the situation where the materialized sql view or the sql view are not exist anymore, so it'll failed on the next refresh by trying delete rows on an nonexistent table.

We met this issue on pg9.2 when we refresh a view that needs to write 4GB temporary files on FS without enough free space.

After add space on the partition click on the refresh button was not able to re-create the materialized view as well. Now we can
we have to fix that issue in a proper way in the concerned module
also rename too long method name
@Cedric-Pigeon
Copy link

@petrus-v Do you plan to finish this PR ? Thanks.

@petrus-v
Copy link
Contributor Author

@Cedric-Pigeon I've done some works last code sprint!

I'm gonna push it right now, helps is welcome however! Today I only use version 7!

@petrus-v petrus-v force-pushed the 8.0-materialized_sql_view branch from 4437caa to 0da27bc Compare October 12, 2016 14:38
Cédric Pigeon and others added 2 commits April 25, 2017 16:42
Copy link
Member

@gurneyalex gurneyalex left a comment

Choose a reason for hiding this comment

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

could you fix the two small issues and rebase the PR?

sql = self.demo_matview_mdl.sql_view_definition
self.demo_matview_mdl._sql_view_definition = None
with self.assertRaises(ValueError):
self.demo_matview_mdl.sql_view_definition
Copy link
Member

Choose a reason for hiding this comment

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

use

sql = self.demo_matview_mdl.sql_view_definition


def setUp(self):
super(AbstractMaterializedSqlViewTester, self).setUp()
self.demo_matview_mdl = self.env['test.materialized.view'].with_context(
Copy link
Member

Choose a reason for hiding this comment

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

line too long (79 char max)

@pedrobaeza
Copy link
Member

This has been stalled for a long time. It's an interesting idea, but it seems unfinished. If someone wants to revive this, please open a new PR.

@pedrobaeza pedrobaeza closed this Apr 28, 2018
@legalsylvain
Copy link
Contributor

legalsylvain commented May 13, 2018

materialized view can now be generated by bi_sql_editor OCA module.

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (10.0)
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.