Skip to content

Add if_not_exists to create_view and if_exists to drop_view#382

Open
serg-kovalev wants to merge 1 commit intoscenic-views:mainfrom
serg-kovalev:feature/add-exist-check-on-view-creation
Open

Add if_not_exists to create_view and if_exists to drop_view#382
serg-kovalev wants to merge 1 commit intoscenic-views:mainfrom
serg-kovalev:feature/add-exist-check-on-view-creation

Conversation

@serg-kovalev
Copy link

@serg-kovalev serg-kovalev commented Feb 8, 2023

Provide an ability to pass if_not_exists param for view creation. It's handy to have it when you disable_ddl_transaction in Rails migration and add indexes concurrently.
And similar functionality with if_exists param to drop a view.

For instance we have a migration like that:

class CreateSomeView < ActiveRecord::Migration[7.0]
  disable_ddl_transaction!

  def up
    create_view :mv_some_name, materialized: true

    safety_assured do
      add_index :mv_some_name, :receipt_uuid, unique: true, algorithm: :concurrently
      add_index :mv_some_name, :invoice_number, algorithm: :concurrently
    end
  end

  def down
    drop_view :mv_some_name, materialized: true
  end
end

As it's recommended to update indexes concurrently out of a transaction, those operations may accidentally fail. It happens very rarely, but we faced it several times. As a result, migration will stuck and it will require manual resolution.

The change that I made doesn't bring some level of uncertainty, I think. As by default, we do everything like we did before, but if we really need to make it a bit flexible, we now can do that.

Please let me know if it makes sense.
Thank you in advance!

@serg-kovalev serg-kovalev force-pushed the feature/add-exist-check-on-view-creation branch from 380eccf to e145024 Compare February 8, 2023 13:30
Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Warning: unrecognized cop Layout/ParameterAlignment found in .rubocop.yml
Warning: unrecognized cop Layout/ParameterAlignment found in .rubocop.yml
Warning: unrecognized cop Layout/AssignmentIndentation found in .rubocop.yml
Warning: unrecognized cop Lint/DuplicateHashKey found in .rubocop.yml
Error: Unknown Ruby version 2.7 found in `TargetRubyVersion` parameter (in .rubocop.yml).
Supported versions: 2.1, 2.2, 2.3, 2.4, 2.5

@serg-kovalev serg-kovalev force-pushed the feature/add-exist-check-on-view-creation branch from 210d552 to f4da7c1 Compare February 8, 2023 13:46
@serg-kovalev
Copy link
Author

@derekprior Hi! Hope you are well.
Did not talk to you since we worked on NO DATA functionality.
Could you please take a look at this PR? What do you think about adding functionality similar to what Rails has?

@serg-kovalev serg-kovalev force-pushed the feature/add-exist-check-on-view-creation branch 2 times, most recently from 6b76770 to 5863bee Compare February 8, 2023 13:56
@derekprior
Copy link
Contributor

Can you say more about this?

It's handy to have it when you disable_ddl_transaction in Rails migration and add indexes concurrently

Conceptually, one of the ideas of Scenic is that careful management of your schema using Scenic means you have certainty around your database state. IF NOT EXISTS implies uncertainty.

@serg-kovalev
Copy link
Author

serg-kovalev commented Feb 9, 2023

Can you say more about this?

It's handy to have it when you disable_ddl_transaction in Rails migration and add indexes concurrently

Conceptually, one of the ideas of Scenic is that careful management of your schema using Scenic means you have certainty around your database state. IF NOT EXISTS implies uncertainty.

@derekprior Sure!
For instance we have a migration like that:

class CreateSomeView < ActiveRecord::Migration[7.0]
  disable_ddl_transaction!

  def up
    create_view :mv_some_name, materialized: true

    safety_assured do
      add_index :mv_some_name, :receipt_uuid, unique: true, algorithm: :concurrently
      add_index :mv_some_name, :invoice_number, algorithm: :concurrently
    end
  end

  def down
    drop_view :mv_some_name, materialized: true
  end
end

As it's recommended to update indexes concurrently out of a transaction, those operations may accidentally fail. It happens very rarely, but we faced it several times. As a result, migration will stuck and it will require manual resolution.

The change that I made doesn't bring some level of uncertainty, I think. As by default, we do everything like we did before, but if we really need to make it a bit flexible, we now can do that.

Please let me know if it makes sense.
Thank you in advance

@serg-kovalev serg-kovalev force-pushed the feature/add-exist-check-on-view-creation branch from 5863bee to b401623 Compare February 14, 2023 09:39
@serg-kovalev
Copy link
Author

@derekprior Hey! Hope you are doing well. Have you had a chance to take a look?

@calebhearth calebhearth added this to the 2.0 milestone Jan 20, 2024
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