-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(schema): check for extension before creating #7002
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
base: main
Are you sure you want to change the base?
Conversation
@ysdanielkim Thank you for bringing this up and submitting a PR no less! I'm reviewing the changes and running them through our internal tests. I'm additionally reviewing how this may have got through our tests which test upgrade scenarios. Can you tell me what version of Postgress you are running? |
@ysdanielkim I don't really understand why the script would make any difference here, you're still calling |
To add to @rodrigozhou 's question: Its unclear what situation this addresses. Can you clarify what situation this addresses? Does it just ensure that the user can check if the extension already exists even if they don't have permission to install extensions? |
@jmbarzee @rodrigozhou Thanks for reviewing. This addresses an issue in Azure PostgreSQL where non-admin users cannot create extensions. The extensions in the database are usually installed by a separate admin user and the application is not given admin rights to the database. This error is returned by Azure PostgreSQL if
And here is the related documentation from Azure. Adding this conditional ensures that And yes, I've tested this on non-azure PG12 as well. |
@ysdanielkim This doesn't seem that will solve anything. The script is for creating necessary table for running Temporal, and we need the extension for that. So, whoever is setting up the DB needs the required permission for running the entire script. The script is meant to be executed only once with the required permissions. |
@rodrigozhou In my project, the application is not given the permission to create the extensions, unfortunately. I'm explicitly requesting that this extension be installed to the database provider. So this extension is already installed when temporal receives a database instance from the database provider. I know this is not a common scenario, but this is necessary to work safely in Azure. 🫤 |
This PR was marked as stale. Please update or close it. |
What changed?
CREATE EXTENSION
sql query is replaced with a pgsql script that checks for the existence of the extension programatically.Why?
CREATE EXTENSION
can cause issues in environment where creating extensions are limited to certain users. (e.g. Azure)How did you test it?
I've ran the updated schema setup in both environments where btree_gin is installed and and not installed.
Potential risks
Regression in visibility database setup.
Documentation
Not needed.
Is hotfix candidate?
No