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

fix: Migration error for Cloud SQL #617

Closed
wants to merge 3 commits into from
Closed

Conversation

nkreiger
Copy link

@nkreiger nkreiger commented Jul 5, 2023

What kind of change does this PR introduce?

Bug fix/Chore.

What is the current behavior?

The function realtime.list_changes is created, and sets the log_min_messages to 'fatal' for each session it runs.

What is the new behavior?

The function realtime.list_changes is created, and if the your permissions are not a super use, it does not set the log_min_messages to 'fatal', but relies on the admin of the DB to set this at the appropriate level

Additional context

This allows the migration to proceed for Cloud SQL setups that do not allow you to have a super user.

@vercel
Copy link

vercel bot commented Jul 5, 2023

@nkreiger is attempting to deploy a commit to the Supabase Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

@olirice olirice left a comment

Choose a reason for hiding this comment

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

would be best to make this a separate migration that create or replaces the function rather than editing an existing migration

EXECUTE 'create or replace function realtime.list_changes(publication name, slot_name name, max_changes int, max_record_bytes int)
returns setof realtime.wal_rls
language sql
set log_min_messages to ''fatal''
Copy link

Choose a reason for hiding this comment

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

if this is the only line of difference, can the if statement be moved into a separate posters query and add/remove this line using string interpolation?

@w3b6x9
Copy link
Member

w3b6x9 commented Jul 6, 2023

@nkreiger thanks for your contribution!

can you also create a fresh migration file and move this in there?

Copy link
Member

@w3b6x9 w3b6x9 left a comment

Choose a reason for hiding this comment

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

@nkreiger
Copy link
Author

nkreiger commented Jul 8, 2023

@w3b6x9 yep, I was going to actually comment that since its the only change maybe it could be broken out. I'll do this now

Signed-off-by: Noah Kreiger <[email protected]>
@nkreiger
Copy link
Author

nkreiger commented Jul 8, 2023

@olirice @w3b6x9 I broke out the steps.

If I were to remove the IF statement entirely from the original migration, then the DB migration would still fail on that step with a permissions error, and I would get stuck there having to manually bump the migration passed it.

Maybe cleaner than the if statement if I just catch and ignore an exception? Let me know if thats preferable or you have alternative ideas.

@olirice
Copy link

olirice commented Jul 10, 2023

If I were to remove the IF statement entirely from the original migration, then the DB migration would still fail on that step with a permissions error, and I would get stuck there having to manually bump the migration passed it.

Maybe cleaner than the if statement if I just catch and ignore an exception? Let me know if thats preferable or you have alternative ideas.

could you please provide the full error message you're seeing?

@nkreiger
Copy link
Author

@olirice sorry I had to pull it out because of the error and haven't had a chance to setup an example environment, will hopefully get back to you asap later this week or early next!

@nkreiger
Copy link
Author

nkreiger commented Nov 1, 2023

@olirice apologies for the delay, this is the error:

03:36:35.796 project=realtime external_id=realtime [error] Error starting Postgres Extention: {:error,
 {:shutdown,
  {:failed_to_start_child, Extensions.PostgresCdcRls.Migrations,
   {%Postgrex.Error{
      message: nil,
      postgres: %{
        code: :insufficient_privilege,
        file: "guc.c",
        line: "8120",
        message: "permission denied to set parameter \"log_min_messages\"",
        pg_code: "42501",
        routine: "set_config_option_ext",
        severity: "ERROR",
        unknown: "ERROR"
      },
      connection_id: 254962,
      query: nil
    },
    [
      {Ecto.Adapters.SQL, :raise_sql_call_error, 1,
       [
         file: 'lib/ecto/adapters/sql.ex',
         line: 932,
         error_info: %{module: Exception}
       ]},
      {Enum, :"-map/2-lists^map/1-0-", 2, [file: 'lib/enum.ex', line: 1658]},
      {Ecto.Adapters.SQL, :execute_ddl, 4,
       [file: 'lib/ecto/adapters/sql.ex', line: 1024]},
      {Ecto.Migration.Runner, :log_and_execute_ddl, 3,
       [file: 'lib/ecto/migration/runner.ex', line: 352]},
      {Ecto.Migration.Runner, :"-flush/0-fun-1-", 6,
       [file: 'lib/ecto/migration/runner.ex', line: 117]},
      {Enum, :"-reduce/3-lists^foldl/2-0-", 3,
       [file: 'lib/enum.ex', line: 2468]},
      {Ecto.Migration.Runner, :flush, 0,
       [file: 'lib/ecto/migration/runner.ex', line: 116]},
      {Ecto.Migration.Runner, :perform_operation, 3,
       [file: 'lib/ecto/migration/runner.ex', line: 289]}
    ]}}}}

let me know what you think! Thanks. In a Cloud SQL instance, it is impossible to achieve these permissions, because its abstracted by the owner (in this case GCP). They provide a cloud sql super role, but it doesn't have the permissions to set this, this is set at the configuration level by terraform, or manually through the UI. So its impossible to avoid this error without updating the DB migration.

@w3b6x9
Copy link
Member

w3b6x9 commented Nov 2, 2023

@nkreiger It's been awhile so just wanted to confirm we're on the same page:

  1. need to update an existing migration file so it runs successfully
  2. create another migration file replacing the function so that it doesn't set log_min_messages on every invocation

We're not going to do step 1 b/c it would change an existing migration file that's already been run on hosted and self-hosted Realtime projects.

We recommend that you move forward with these migrations on your forked Realtime so you're not blocked when using it against a Cloud SQL database.

@w3b6x9 w3b6x9 closed this Nov 2, 2023
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