temporalio / temporal

Temporal service
https://docs.temporal.io
MIT License
11.26k stars 810 forks source link

Schema name selection for postgres #3383

Open vinodnaidu02 opened 1 year ago

vinodnaidu02 commented 1 year ago

Is your feature request related to a problem? Please describe. Trying to set schema namespace which temporal has to use but not able to set using sql-tool.

Describe the solution you'd like based on the search path selection, temporal has to use that schema.

alexshtin commented 1 year ago

Sounds like a good idea. Are you willing to contribute? Schema file is here: https://github.com/temporalio/temporal/blob/master/schema/postgresql/v96/temporal/schema.sql Queries are here: https://github.com/temporalio/temporal/tree/master/common/persistence/sql/sqlplugin/postgresql (almost in every file).

Visibility schema/queries are also needs to be updated.

vinodnaidu02 commented 1 year ago

Hi Alex

I am able to set schema while using sql-tool. I used the below command while installing. The only problem is there is no documentation that explains how to use search_path,

 SQL_DATABASE=temporal ./temporal-sql-tool --ca search_path=temporal setup-schema -v 0.0

The main issue I faced was configuring the temporal server to use this schema, I also found a vague solution to fix this. still I am not very sure if this is proper,

connectAttributes: 
                    search_path:  "{{ default .Env.SCHEMA_NAME "temporal" }}"

The above config value is part of Mysql config_template, however, for Postgres, there is no option. By adding this line to both temporal and visibility databases in the Postgres part, the temporal server is connecting to the respective schema.

Please let me know if you want I can do these changes(Hopefully my approach is correct).

anjulaendowus commented 1 year ago

@vinodnaidu02 Anything changed with this approach? Im facing this same problem.

vinodnaidu02 commented 1 year ago

@anjulaendowus I am still using the above approach I mentioned. I created a docker image based on the above config which accepts schema ENV and uses that while connecting.

anjulaendowus commented 1 year ago

@vinodnaidu02 DId u use this command? SQL_DATABASE=temporal_visibility ./temporal-sql-tool update -schema-dir schema/postgresql/v96/visibility/versioned did you pass the --ca flag as well for this? Does this work with postgres? Thanks for the reply

vinodnaidu02 commented 1 year ago

Yes I did use the command and pass the --ca flag and it is working with the Postgres. Not sure if something has changed recently.

anjulaendowus commented 1 year ago

Did u manually create the schema @vinodnaidu02 ?

vinodnaidu02 commented 1 year ago

Yes, I manually created the schema inside DB before temporal starts.

chlunde commented 6 months ago

https://www.percona.com/blog/public-schema-security-upgrade-in-postgresql-15/

So I think we need a CREATE SCHEMA IF NOT EXISTS schema_name (which is manual today), in addition to the search_path (which is possible to set, but only as an attribute which means that temporal doesn't really know what's going on).

@alexshtin do you have any input on how to implement this? I'm not familiar with this code base, so I wonder what's the natural place to:

  1. Define the schema name as a property (for templating the above query below and possibly setting search_path automatically)
  2. Put the CREATE statement. Maybe after CREATE DATABASE (but I would like an IF NOT EXISTS on that too, because the database provider might have created the db but not the schema, or neither db and schema).
bkcsfi commented 6 months ago

I encountered a problem today with temporal-sql-tool not working with postgres when the database already exists and the db user is NOT a superuser.

I am using temporal-operator in conjunction with cloudnative-pg. The later handles database creation, replication and lifecycle operations. Therefore the database already exists by the time the temporal-sql-tool create operation is executed by the temporal-operator.

In addition, cloudnative-pg no longer creates users with superuser access.

The temporal-sql-tool postgres module checks for "duplicate database" error AFTER attempting to create the database. However since the user does not have superuser access, temporal-sql-tool create fails with pq: permission denied to create database

Is this a reasonable issue on which to graft "check for existing database before attempting to create"?

If that change were implemented, its possible that a non-superuser database account would work properly with temporal-sql-tool. (though I don't know what roles the schema upgrade steps require).

hansliu commented 2 months ago

Could we add the connectAttributes: search_path to the default config_template.yaml?

Or how to override the config_template.yaml from docker hub temporalio/server image?