Extend configuration to support disable_during_db_migrate

[bschrag620]

As mentioned in #33, parsing of the sql messages can have a large impact on the schema dump process when running rails db:migrate. The intent of this change is to allow for a configurable option to allow users/teams to opt out of the formatting during the migration process. This allows for consumers of pp_sql to leverage it for formatting within their environment without having to suffer the performance cost when migrating their database.

To opt-in, simply add PpSql.disable_during_db_migrate = true within the specific envrionment file or within the initializer. By opting into disable_during_db_migrate, a before hook will be ran in front of db:migrate task that will set both rewrite_to_sql_method and add_rails_logger_formatting to false for the duration of the task.

---

Important

Add disable_during_db_migrate option to PpSql to disable SQL formatting during rails db:migrate, with documentation updates.

• Behavior:
  • Adds disable_during_db_migrate option to PpSql in lib/pp_sql.rb to disable SQL formatting during rails db:migrate.
  • New Rake task pp_sql:disable_during_db_migrate in db_hooks.rake to set add_rails_logger_formatting and rewrite_to_sql_method to false if disable_during_db_migrate is true.
• Documentation:
  • Updates README.md to include instructions for using disable_during_db_migrate option.

^{This description was created by for 8a78a4e. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 8a78a4e in 2 minutes and 10 seconds. Click for details.

• Reviewed 64 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. README.md:72

• Draft comment:
  Consider expanding the explanation in this new 'Disable during db:migrate' section. A brief note on why and when you might want to disable formatting could help users.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50% None

2. lib/pp_sql.rb:9

• Draft comment:
  Add a brief comment for the new 'disable_during_db_migrate' flag to explain its purpose, similar to the other flags.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

3. lib/pp_sql/tasks/db_hooks.rake:6

• Draft comment:
  Consider logging a message when pp_sql formatting is disabled during db:migrate to aid in debugging.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

4. lib/pp_sql/tasks/db_hooks.rake:4

• Draft comment:
  Typo: In the description string, "Hook that is ran before rails db:migrate...", 'ran' should be changed to 'run'.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammar correction is technically correct, this is just a task description string that developers will rarely see. It doesn't affect functionality. The meaning is still perfectly clear despite the minor grammatical error. The rules say to only keep comments that require clear code changes, and to avoid unimportant changes. The grammar mistake could be seen as unprofessional, and fixing it would improve code quality. Documentation is also code. While documentation quality matters, this is an extremely minor issue that doesn't impact understanding or functionality. The rules specifically say to avoid obvious or unimportant comments. This comment should be deleted as it suggests a trivial change that doesn't meaningfully impact code quality or functionality.

Workflow ID: wflow_CoMhZmbANoa2SAyV

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[kvokka]

Thank you for your contribution!

Regarding the case you are covering, i suppose that the migrations are not the only Rails rake tasks that might be affected by this problem, and other DB operations are at risk. May I ask you to extend that to dynamically loaded Rails db:* tasks, and enable this feature by default?

With the enhancement mentioned before, it might require improving the naming of the function to be more generic, since it'll be not only about the migrations. and add an env var trigger to that, something like PPSQL_DISABLED, giving the user the ability to control the process.

The readme section will also require the changes, which should be done under Rails section, since it's the code that will be used by Rails users only.

wdyt about this?

[bschrag620]

Thanks for the quick response!

With the enhancement mentioned before, it might require improving the naming of the function to be more generic, since it'll be not only about the migrations. and add an env var trigger to that, something like PPSQL_DISABLED, giving the user the ability to control the process.

Ah, so you are suggesting that PPSQL_DISABLED=1 rails db:migrate allow for opt-out at runtime. Is that correct? But still retaining the configurable option?

[bschrag620]

@kvokka I've made some changes but not yet tested. Let me know what you think. I had the thought that maybe your suggestion regarding ENV['PPSQL_DISABLE'] is that it should be checked inside the methods doing the formatting. Let me know if that's what you are intending and I can move it upstream.

[bschrag620]

@kvokka I've tested the latest version with some different combinations. I used the following script in my local project:

#!/bin/bash

# This represents the current pp_sql gem
echo "Running on master..."
git co pp-sql-master && bundle install
time1=$( { time rails db:migrate; } 2>&1 | grep real | awk '{print $2}' )

# The branch used here has the gemfile updated to use the latest commit from this branch
echo "Running on bschrag620:pp_sql"
git co pp-sql-no-changes && bundle install
time2=$( { time rails db:migrate; } 2>&1 | grep real | awk '{print $2}' )

# This branch set `Ppsql.disable_for_db_tasks = false` in development.rb - the timing of this result demonstrates pp_sql is being used to format the logs
echo "Running on bschrag620:pp_sql with db_tasks enabled"
git co pp-sql-db-enabled
time3=$( { time rails db:migrate; } 2>&1 | grep real | awk '{print $2}' )

# Using the same branch as above, but no overriding with the env var
echo "Running on bschrag620:pp_sql with db_tasks enabled and ENV override used"
time4=$( { time PPSQL_DISABLED=1 rails db:migrate; } 2>&1 | grep real | awk '{print $2}' )

echo "Results:"
echo "On master:                                        $time1"
echo "On bschrag620:pp_sql:                             $time2"
echo "On bschrag620:pp_sql tasks enabled:               $time3"
echo "On bschrag620:pp_sql tasks enabled env override:  $time4"

Here are the results:

Results:
On master:                                      0m59.531s
On bschrag620:pp_sql:                           0m12.853s
On bschrag620:pp_sql tasks enabled:             0m50.707s
On bschrag620:pp_sql tasks enabled env override:        0m11.568s

There is still an outstanding TODO here to document usage of the env var in the readme (as well as correct some of the other newly added info?). However, I wanted to make sure these changes are inline with what you were requesting before making the final readme change.

[kvokka]

Please keep in mind the changes that I left in the review comments, which will provide the same functionality, but by extending the code only.

[kvokka]

it's a bit odd, since with this lately u'r wrapping the boolean settings with another one. Seems to be excessive

[bschrag620]

Since I was originally using a disabled_ accessor the logic was feeling hard to read (if enabled && !ENV[disabled]) so pulling it into a single method felt cleaner. However, based on your other suggestions the disabled ENV can go away so I'll remove this method.

[kvokka]

      rake_tasks do
        path = File.expand_path(__dir__)
        load("#{path}/pp_sql/tasks/db_hooks.rake") unless PpSql.enable_for_rails_rake_db_tasks
     end

[kvokka]

  task :disable_during_db_tasks do
    PpSql.add_rails_logger_formatting = false
    PpSql.rewrite_to_sql_method = false
  end
end

[kvokka]

  self.enable_for_rails_rake_db_tasks = ENV['PPSQL_ENABLE_FOR_RAILS_RAKE_DB_TASKS']

[bschrag620]

From testing, something I didn't quite realize is that this file and the rake tasks are loaded before any of the local project files/initializers are read. This means that setting enable_for_rails_rake_db_tasks within the project won't have any effect because the rake task loading will have already been executed (or skipped). In other words, with the current changes, the only way to opt into formatting is by usage of the ENV var. Is that ok?

[bschrag620]

@kvokka Latest testing round, slightly updated script:

#!/bin/bash

echo "Running on master..."
git co pp-sql-master && bundle install
time1=$( { time rails db:migrate; } 2>&1 | grep real | awk '{print $2}' )

echo "Running on bschrag620:pp_sql"
git co pp-sql-no-changes && bundle install
time2=$( { time rails db:migrate; } 2>&1 | grep real | awk '{print $2}' )

echo "Running on bschrag620:pp_sql with db_tasks enabled via ENV var"
time3=$( { time PPSQL_ENABLE_FOR_RAILS_RAKE_DB_TASKS=1 rails db:migrate; } 2>&1 | grep real | awk '{print $2}' )

echo "Results:"
echo "On master:                                        $time1"
echo "On bschrag620:pp_sql:                             $time2"
echo "On bschrag620:pp_sql tasks enabled:               $time3"

Results:

Results:
On master:                                      0m44.440s
On bschrag620:pp_sql:                           0m10.883s
On bschrag620:pp_sql tasks enabled:             0m44.560s

[bschrag620]

@kvokka I wanted to check in and see if there are other changes you would like on this branch that could unblock it. Thanks!