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

Execute SQL only with option --live #50

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tolq15
Copy link

@tolq15 tolq15 commented Mar 30, 2019

The reason behind this change:
I have several databases with pretty big numbers of users, schemas and tables.
At first, I've run pgbedrock with option 'generate' to generate configuration files. Then I've run pgbedrock with 'configure --check' parameter with output redirected to file. Practically all Postgres session hanged up. When I killed such sessions (using pg_terminate_backend), I found file generated by pgbedrock with pretty big number of SQL commands (up to 300.000 lines).
It were commands like
grant REFERENCES/TRIGGER/TRUNCATE ...
and
alter DEFAULTE PRIVILEGE ... TO <superuser_name>
I supposed that the problem was in 'rollback' all these command.
To fix this, I tried to skip execution of these commands if parameter 'live' set to 'False'.
My tests showed that this really fix the problem, and all output files were generated as expected.
This change also make pgbedrock run faster with '--check' option because we do not need to execute generated SQL statements and then roll them back.
I hope this change will make pgbedrock a little bit better.

Add parameter 'live' to function run_module_sql() to execute SQL only if pgbedrock was started with option --live.
This can prevent unnecessary SQL execution and rollback.
This change also fix problem with hanged up Postgres session when there are a lot (more then ~10.000) of generated SQL statements.
@coveralls
Copy link

coveralls commented Mar 30, 2019

Pull Request Test Coverage Report for Build 183

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.002%) to 97.233%

Totals Coverage Status
Change from base Build 179: -0.002%
Covered Lines: 1265
Relevant Lines: 1301

💛 - Coveralls

Fix error in the previous commit. Parameter 'live' was added to analyze_ownerships function. It was a mistake.
@zcmarine
Copy link
Contributor

The issue with this is that the changes that each pgbedrock module determines are influenced by the changes that the prior modules make. For example, the analyze_privileges module runs last and will grant, revoke, or do nothing on a table if you own it, and that ownership is based on the analyze_ownerships module that runs before it. It's because these depend on each other that everything is run in a transaction in check mode: so we get an accurate representation of the changes that will be needed.

To avoid things clogging up because of too many commands being rolled back, maybe you could run pgbedrock configure --check with only the first module (attributes), then if it looks good productionize it, then do the same step-by-step with the subsequent modules. Alternatively I could see supporting this no-transaction approach shown in this PR but only if we're running configure --check with only a single module, since otherwise we can't guarantee that the results are correct.

I'll leave this PR open in case you want to try going one of those routes.

@tolq15
Copy link
Author

tolq15 commented Apr 15, 2019 via email

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.

3 participants